-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
Fix implemented for making it work when using NodeJs module.createRequire Signed-off-by: inglkruiz <12603425+inglkruiz@users.noreply.github.com>
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
Some context about this change. I'm using NodeRed for wiring a data flow and I'm using this package, recently I made a PR for fixing an issue I found after one of my flows stopped working https://github.com/node-red/node-red/pull/3984/files I did read about ESM and I understand the extension for Also I understand the package has been moved to be ESM only but many of us I believe cannot migrate right now to ESM, multiple reasons could be named, but mine is that it would take a huge effort to upgrade NodeRed to ESM, moreover, I'm not a contributor, and they have implemented a fix to overcome this > node-red/node-red#3645 Now, I also have the same issue these packages
Lastly, I have to admit that I'm not sure if this is the way to fix it but I can say my flows in NodeRed stopped throwing errors. |
@achingbrain Hi, could you or any of the contributors review this PR? Feedback is very welcome. Also, if this does not align with the repo's vision let me know how ppl not using ESM could start using the newest version? |
Thanks for submitting this PR but it's not going to solve the problem - by adding
If you need to import ESM from CJS you need to use the dynamic import function. See: #4256 |
Looking at the code in NodeRed they indeed use the dynamic import function, see > https://github.com/node-red/node-red/pull/3645/files#diff-785d7569067dc564a5a97de5578752c3ad37a42a213b55dd1956081c32294f68R147-R151 The error I would say is when they call: // To handle both CJS and ESM we need to resolve the module to the
// specific file that is loaded when the module is required/imported
// As this won't be on the natural module search path, we use createRequire
// to access the module
const modulePath = createRequire(moduleDir) I debugged the code and there is where it breaks. Would you need a repo for reproducing it? ThIs package and NodeRed is very important to us (at the company where I work) since it allows to integrate different technologies using a data flow. Our users like it and it would be sad if we have to stay in one of js-ipfs old version. |
I don't think that code will work as intended as it's using Does it work if you change it to just: return import(module); |
Fix implemented for making it work when using NodeJs module.createRequire. FYI > https://nodejs.org/api/module.html#modulecreaterequirefilename
Signed-off-by: inglkruiz 12603425+inglkruiz@users.noreply.github.com