-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix package exports field #64
Conversation
2 questions:
|
I checked out this PR branch and built the package, and this is the result from the website you mention above: Also, with the changes you suggest nodejs no longer imports the correct package (it imports the package from the esm folder). I would really like to support using the modern |
Thanks for having a look.
There's a CLI, so we could probably set something up, yes.
This feels like a good reference, reviewed by authoritative people. I came across it from this article, which lists other resources.
I think the latest versions of Node are meant to import the ESM entrypoint of the package; that's the whole point. So maybe it's a matter of reviewing how that entrypoint is generated? I'll take a deeper look and get back to you. |
Currently three entrypoints are generated (including two different ESM):
No CJS is currently built, but it's a one-liner to add it and it might be a good idea if some node environments want to use it with |
Right, okay, so perhaps something like this would work?
I don't quite understand how the |
At build time, these link flags are used for the browser builds:
while these are used for the nodejs build:
|
If I make a CJS build, it's not clear to me if it should target nodejs or the browser. CJS is not the default for node anymore, thought it is still common - does nextjs use CJS to build browser pages? That seems likely to change soon, too. |
I don't think CJS builds are needed; NextJS just needs ESM, and all the recent bundlers and tools support it. The problem comes down to allowing users to import the platform bundle they need explicitly (i.e. either the Node bundle or the browser bundle), because only they know which one they need. This can be achieved by exposing two
Then we can decide which format (ESM/CJS) to support for each export by providing the corresponding keys ( |
Note that we could also have a dedicated worker bundle:
|
Of course, I come at this with the optics of using h5wasm in a front-end app. If you prefer, it's also possible to make the Node import the default and the browser import explicit - e.g. |
3e44854
to
bf89b2e
Compare
"type": "module", | ||
"main": "./dist/node/hdf5_hl.js", | ||
"main": "./dist/iife/hdf5_hl.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems unpkg and jsdelivr both fall back to the browser
field and/or the main
field, so I'm putting the IIFE build in there. This should allow shortening the URL, for instance:
self.importScripts('https://cdn.jsdelivr.net/npm/[email protected]');
Also - I apologize for the lack of documentation on getting a working build environment for h5wasm. It has gotten much easier lately, if you're using linux: I recommend using conda: conda install --channel=conda-forge emsdk cmake make
emsdk install 3.1.43
emsdk activate
# then activate the tools with the instructions it prints out for emsdk_env.sh
cd h5wasm
make clean
make
npm run build
npm test ... and that's it! |
Here is your PR branch built into a package (after renaming the import in all the tests from 'h5wasm' to 'h5wasm/node') - it passes all tests and seems to work fine, so please check if it works for your build system too. |
No worries, I had managed to install everything, but then was running into permission issues when running
I've tested in both Node and NextJS and I'm not seeing any problems any more. I've also updated the import paths in the tests and they pass as you mentioned. 🎉 |
Attempt at fixing an issue with importing h5wasm in NextJS: silx-kit/h5web#1525.
I've used arethetypeswrong to diagnose the problem and this guide to fix it.
NextJS was using the
CommonJS exportESM Node bundle instead of the ESM browser bundle.The problem is thatsee #64 (comment)node
is not a validexports
entry; it should berequire
(along withimport
which is already present). Vite and webpack do not seem to mind, so I assume they skip the unknownnode
entry. The recommendation is also forrequire
to come afterimport
, but I'm not sure this really matters in practice.I'm not sure this PR will make
h5wasm
pass all the arethetypeswrong checks. I'm struggling to buildh5wasm
on my machine, so I can't upload a tarball to arethetypeswrong to check.