ipfs / js-ipfs

IPFS implementation in JavaScript
https://js.ipfs.tech
Other
7.43k stars 1.25k forks source link

fix: ipfs-http-client exports #4265

Closed inglkruiz closed 1 year ago

inglkruiz commented 1 year ago

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

welcome[bot] commented 1 year ago

Thank you for submitting this PR! A maintainer will be here shortly to review it. We are super grateful, but we are also overloaded! Help us by making sure that:

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. Next steps:

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. We are very grateful for your contribution!

inglkruiz commented 1 year ago

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 require should be .cjs, although I tested this locally changing the package.json at node_modules/ipfs-http-client and the local instance stopped throwing the following error

image

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 > https://github.com/node-red/node-red/pull/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.

inglkruiz commented 1 year ago

@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?

achingbrain commented 1 year ago

Thanks for submitting this PR but it's not going to solve the problem - by adding require to the exports map and pointing it at an ESM file, it will cause use of require to attempt to load the ESM file.

require does not work with ESM files, only CJS so this will fail with module not allowed as you have seen in your own local testing.

If you need to import ESM from CJS you need to use the dynamic import function.

See: https://github.com/ipfs/js-ipfs/issues/4256

inglkruiz commented 1 year ago

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.

achingbrain commented 1 year ago

I don't think that code will work as intended as it's using require.resolve which only works with modules that have a 'main' field in their package.json and not the newer exports field.

Does it work if you change it to just:

return import(module);