replace node-fetch with native fetch module (coming with node v18)
Closed this issue · 24 comments
We are using node-fetch for fetching url's in 3 places:
- calendar
- newsfeed
- e2e tests
With the new version v3 they decided to convert node-fetch to ES module so we can not use require anymore. I testet the alternative syntax using const fetch = (...args) => import('node-fetch').then(({default: fetch}) => fetch(...args)); instead.
The modules calendar and newsfeed are working with this change, but jest is failing with a Segmentation fault:
PID 2750 received SIGSEGV for address: 0x10
/opt/magic_mirror/node_modules/segfault-handler/build/Release/segfault-handler.node(+0x3206)[0x7f18049f3206]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x12730)[0x7f18046b9730]
node[0xa15ec0]
node(_ZN2v88internal7Isolate38RunHostImportModuleDynamicallyCallbackENS0_6HandleINS0_6ScriptEEENS2_INS0_6ObjectEEENS0_11MaybeHandleIS5_EE+0xad)[0xe4bb5d]
node(_ZN2v88internal25Runtime_DynamicImportCallEiPmPNS0_7IsolateE+0xb1)[0x11fd7d1]
node[0x15ce134]AFAIS this error comes from the headless chromium browser used for the e2e tests.
At this point I stopped my PR for using node-fetch v3. They still support v2 with security patches, so we have no problem yet.
For the future we need a solution for this or have to use antoher library.
In package.json node-fetch must stay on v2.
Thanks for the investigation. I agree to keep v2.
Converting to es modules .... That might help the overall project but has a loooot of implications :-) Future selfs problem I'd say ;-)
@khassel I had a look into your MR. I'm not sure why we spin up the full electron app for those tests. It should be sufficient there to only run in server mode only.
I applied your recent changes from https://github.com/MichMich/MagicMirror/pull/2655/files to your branch node-fetch.
I added the import const app = require("../../js/app"); to the vendor_spec.js file.
Changed the beforeAll hook to:
beforeAll(function (done) {
app.start(() => {
done();
});
});
I received the error:
Cannot find module 'logger' from 'js/app.js'
Require stack:
js/app.js
tests/e2e/vendor_spec.js
However, Jest was able to find:
'./logger.js'
Therefore I also applied the moduleNameMapping in the package.json for the e2e tests.
Then I got TypeError: logger_1.default is not a function.
I'm not sure who would trigger the default, so I added a default to the logLevels default: Function.prototype.bind.call(console.log, console).
However, it still complains about default is not a function.
Any ideas on how to fix the logger import?
First of all I'm not very familiar with these e2e tests.
I'm not sure why we spin up the full electron app for those tests. It should be sufficient there to only run in server mode only.
agree to minimize use of e2e
Then I got TypeError: logger_1.default is not a function.
can reproduce this, but I think this error message is misleading because of the following lines:
TypeError: logger_1.default is not a function
at Object.<anonymous> (node_modules/@wdio/utils/build/initialiseServices.js:9:29)
at Object.<anonymous> (node_modules/@wdio/utils/build/index.js:9:30)The error is related to @wdio and this webdriverio error is coming from stuff in required global-setup.js where more electron and spectron things are done. May this helps for digging deeper ...
Tested with the current develop branch the following setup:
Added moduleNameMapper to the e2e section in package.json
"moduleNameMapper": {
"logger": "<rootDir>/js/logger.js",
"node_helper": "<rootDir>/js/node_helper.js"
},Changed the vendor_spec.js file to:
const fetch = require("node-fetch");
const app = require("../../js/app.js");
describe("Vendors", function () {
beforeAll(function () {
process.env.MM_CONFIG_FILE = "tests/configs/env.js";
app.start();
});
afterAll( function () {
app.stop();
});
describe("Get list vendors", function () {
const vendors = require(__dirname + "/../../vendor/vendor.js");
Object.keys(vendors).forEach((vendor) => {
it(`should return 200 HTTP code for vendor "${vendor}"`, function (done) {
const urlVendor = "http://localhost:8080/vendor/" + vendors[vendor];
fetch(urlVendor).then((res) => {
expect(res.status).toBe(200);
done();
});
});
});
Object.keys(vendors).forEach((vendor) => {
it(`should return 404 HTTP code for vendor https://localhost/"${vendor}"`, function (done) {
const urlVendor = "http://localhost:8080/" + vendors[vendor];
fetch(urlVendor).then((res) => {
expect(res.status).toBe(404);
done();
});
});
});
});
});Executing this test with npx jest tests/e2e/vendor_spec.js --force-exit works (except the process needs to be force exited).
But running with node-fetch v3 I'm getting again a segmentation fault.
Edit: Its running with v3 using NODE_OPTIONS=--experimental-vm-modules npx jest tests/e2e/vendor_spec.js --force-exit.
So this is may no solution for the node-fetch upgrade but I will check which other e2e tests could be run with this method which means without the electron/spectron stuff.
I created a PR #2750.
Since node-fetch is converted into an ES module, a simple require statement no longer works.
I also tested the cross-fetch module. It worked and I thought it was a good solution until I found out that it also depends on node-fetch 2.6.1.
Oh, I missed that @khassel already tested the alternative syntax 😑 So this is not a step forward, sorry.
problems are the tests, mm itself is running with this change but the tests are failing with a segmentation fault, e.g. npx jest tests/e2e/env_spec.js.
Seems to be a node issue so we may have to wait until that is fixed on their side.
Okay, I don't think there is an urgent need for us to upgrade now. Should I close my PR?
I did the same work already in September ;) but decided not to create a PR. If you want to leave your PR open you can look into mine to solve the lint problem (instead of ignoring the files) and I also had to change the digest-fetch import but I forgot meanwhile if this is necassary ...
digest-fetch still depends on node-fetch 2.x. So it makes sense that you replaced it too.
I'll leave my PR open, if it bothers someone, we can of course close it.
I have started to replace node-fetch with the build in package https, but it is more complex and I am not sure whether I will finish it.
I'll leave my PR open, if it bothers someone, we can of course close it.
you could convert it to draft so it is clear that it should not merged at the moment
I have started to replace node-fetch with the build in package https, but it is more complex and I am not sure whether I will finish it.
inspired of your answer I did this yesterday in my module MMM-RepoStats, here the diffs.
Beside this is working in the module I think it is no good idea to do this here. I ran in 2 problems
httpssupports no redirects out of the box, you need an extra package. One of my url's needed a/at the end, without it was redirected and therefore didn't work.httpssends no defaultUser-Agentheader. One of my url's didn't work without.
In my case this is not problematic because the url's are fix but in mm the url's for calendar or newsfeed are defined by the user. Therefore, I consider a well-maintained package like node-fetch, got or axios to be a better choice.
I'm not sure if it's worth bringing this up here or on another ticket, but moving from requests to node-fetch caused some plugin modules to fail, as they required the requests module. @KristjanESPERANTO suggests that they have started some work on "converting" node-fetch to built-in https. This should be considered a breaking change, since modules will then be required to install and use node-fetch on their own, similar to what has to be done for some current third party modules and the requests node module.
My suggestion would be to enhance node_helper with some async functions to aid in basic REST functionality. What that interface looks like, however, I'm not really sure. In that case, rather than having to invoke the library directly, third party devs can use the node_helper functions to retrieve HTML. It looks like there has been some start to this with checkFetchStatus and checkFetchError being added to the node helper already.
This would allow third party devs to use node helper for data retrieval, and allow core devs some ability to ensure consistency in error handling during data retrieval.
Also, for what it's worth, I'd be willing to put some time into both an ES module migration and a typescript migration, but would probably need the help of those familiar with the nitty-gritty to get in there without messing too much up.
but moving from requests to node-fetch caused some plugin modules to fail, as they required the requests module.
this is only correct for those modules, which didn't define their dependencies in a package.json, for more info see #2617 .
suggests that they have started some work on "converting" node-fetch to built-in https.
see my comment above "no good idea" ...
My suggestion would be to enhance node_helper with some async functions to aid in basic REST functionality.
we can discuss this but @MichMich is the one who decides this, so I would wait for his statement before someone begins with coding.
@khassel I don't plan on doing anything without consulting.. it's definitely an expanded use of the node_helper which may add unnecessary overhead to non-data plugins.
While the module development section on the website outlines how to use native modules, it doesn't really explicitly say "Don't use modules that we already use" and, because of how JS is loading modules, if it's there, it can be used. Perhaps something as simple as a nice large warning for module devs to warn them to always install the packages they need via their own packages.json and to not assume packages and versions within magicmirror will be available.
What do you think about using node-fetch-commonjs? I have put it in #2750 as a test.
I think it is better than continuing to use node-fetch 2.x, but technically it is not really optimal.
Are there any features in v3 that are missing for use in this project? The node-fetch team still provides fixes for the v2 of the module.
The nodejs team also announced that they implement fetch as a native module in the next LTS version 18 nodejs/node#41749 which can already be used with the experimental flag.
I'm not sure if it is worth it to switch now to another module without a need instead of updating v2 with the fixes and then switching for the native module.
That fetch becomes a native module in nodejs was news to me. Thanks! I think we can wait for that and as you suggested then switching to the native module 🙂
Will take a while until node 18 will be the minimum version for MM² but I agree, until then we can use v2 of the library
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
should not be closed.
If you remove node-fetch from your external libraries (or convert to the ES module), there will be a knock-on effect for many module developers: a number of modules current have something like this at the top of their node_helper.js:
const fetch = require("node-fetch");It is understandable (and desirable) to continue to upgrade as new functionality becomes available. My only ask is that, if and when these types of changes occur, some effort is made in either the forums or discord (or both) to notify module developers of the changes, it will be helpful in order to update our modules to be compatible.
That said, given that the http fetch functionality seems to be very common but also constantly changing, does it make sense to extend the node_helper.js class to include a fetch function or, at the least, return a known fetch client that would allow us to retrieving data via http calls? Would the community consider, perhaps, using a different third party library (such as Axios) as the default within the node helper?
I understand the desire to allow module developers the ability to use their own libraries/make their own calls, but it would seem some modules have been built to utilize modules installed by MagicMirror Core. This can present difficulties, particularly during troubleshooting, because every module does things differently, with some self-contained (not using dependencies from the core) and others not self-contained.
I'd certainly be willing to give this type of change a go.
sorry for the late reply.
If you remove node-fetch from your external libraries
at the moment we have to stay with node-fetch v2 because the internal fetch function included in Node > v18 was not stable in our tests.
My only ask is that, if and when these types of changes occur, some effort is made in either the forums or discord (or both) to notify module developers of the changes, it will be helpful in order to update our modules to be compatible.
we can do this (if we switch in the future) but this will reach only a few active developers. We had already such a situation when we removed request. These old modules are still in use but never fixed which caused a lot of support in the forums.
does it make sense to extend the node_helper.js class to include a fetch function
we have already such a fetch function in js/fetch.js which is just a wrapper around node-fetch at the moment (we left the part where internal fetch was used as comment). I never tested this but this should work in modules too.
I understand the desire to allow module developers the ability to use their own libraries/make their own calls, but it would seem some modules have been built to utilize modules installed by MagicMirror Core.
I don't see anything we can do to prevent this ...
one should note that developers 'utilize modules installed by MagicMirror Core.' is because we don't have professional developers and they are cloning the default modules and re-using the code.. this is a good OS model.
i have added code to my upgrade script to try to address the modules where developers didn't know about package.json (as it wasn't needed for default modules) etc,etc,etc..
part of the lifecycle
I don't see anything we can do to prevent this ...
nor should we.. but, we should try to cover our tracks.. the default modules should have documentation on their used libs and package.json files.. (even if they aren't actively 'used')