Skip to content
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

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Nov 24, 2023

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.

image

NextJS was using the CommonJS export ESM Node bundle instead of the ESM browser bundle. The problem is that node is not a valid exports entry; it should be require (along with import which is already present). Vite and webpack do not seem to mind, so I assume they skip the unknown node entry. The recommendation is also for require to come after import, but I'm not sure this really matters in practice. see #64 (comment)

I'm not sure this PR will make h5wasm pass all the arethetypeswrong checks. I'm struggling to build h5wasm on my machine, so I can't upload a tarball to arethetypeswrong to check.

@bmaranville
Copy link
Member

2 questions:

  1. is there a way to test whatever arthetypeswrong is checking for in our github actions? I would prefer important tests to occur automatically in CI
  2. I don't know of an authoritative reference on how to construct a package.json that will work for node, esm and CJS - do you know where to find one? No offense to https://github.com/frehner/modern-guide-to-packaging-js-library#define-your-exports but I don't see where they got their information from.

@bmaranville
Copy link
Member

I checked out this PR branch and built the package, and this is the result from the website you mention above:
image

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 import statement rather than require for nodejs users. I don't know what NextJS is doing under the hood, but I think nodejs uses exports.node. Esbuild lists node as a valid key for export in package.json as well: https://esbuild.github.io/api/#conditions

@axelboc
Copy link
Contributor Author

axelboc commented Dec 4, 2023

Thanks for having a look.

is there a way to test whatever arethetypeswrong is checking for in our github actions? I would prefer important tests to occur automatically in CI

There's a CLI, so we could probably set something up, yes.

I don't know of an authoritative reference on how to construct a package.json that will work for node, esm and CJS - do you know where to find one? No offense to https://github.com/frehner/modern-guide-to-packaging-js-library#define-your-exports but I don't see where they got their information from.

This feels like a good reference, reviewed by authoritative people. I came across it from this article, which lists other resources.

Also, with the changes you suggest nodejs no longer imports the correct package (it imports the package from the esm folder).

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.

@bmaranville
Copy link
Member

Currently three entrypoints are generated (including two different ESM):

  • an ESM for the browser (with MEMFS, IDBFS and WORKERFS)
  • a compatibility IIFE built from the browser ESM (use with <script src=""> tags)
  • an ESM for nodejs, which has access to the raw native filesystem by default (NODEFS, not compatible with a browser)

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 require()

@axelboc
Copy link
Contributor Author

axelboc commented Dec 4, 2023

Right, okay, so perhaps something like this would work?

"main": "...", // ESM browser bundle (since there's no CJS build)
"module: "...", // ESM browser bundle also
"types": "...", // TypeScript declarations
"browser: "...", // IFEE/UMD browser bundle
"unpkg": "...", // IFEE/UMD browser bundle
"exports": {
  ".": { "types": ..., "import": ... } // ESM browser bundle and types: `import h5wasm from 'h5wasm';`
  "./node": { "types": ..., "import": ... }, // ESM Node bundle and types: `import h5wasm from 'h5wasm/node';`
}

I don't quite understand how the build_esm and build_node scripts lead to having bundles with different Emscripten file systems. Would you mind clarifying this for me, please, just for my own curiosity?

@bmaranville
Copy link
Member

At build time, these link flags are used for the browser builds:

    -lidbfs.js \
    -lworkerfs.js \
    -s ENVIRONMENT=web,worker \

while these are used for the nodejs build:

    -s NODEJS_CATCH_EXIT=0 \
    -s NODEJS_CATCH_REJECTION=0 \
    -s NODERAWFS=1 \
    -s ENVIRONMENT=node \

@bmaranville
Copy link
Member

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.

@axelboc
Copy link
Contributor Author

axelboc commented Dec 4, 2023

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 exports paths in package.json as suggested above:

  • "." for the browser bundle: import h5wasm from 'h5wasm';
  • "./node" for the Node bundle: import h5wasm from 'h5wasm/node';

Then we can decide which format (ESM/CJS) to support for each export by providing the corresponding keys (import/require) — but as mentioned, I think only ESM is needed, so just import (and types of course).

@axelboc
Copy link
Contributor Author

axelboc commented Dec 4, 2023

Note that we could also have a dedicated worker bundle:

"exports": {
  ...
  "./worker": {
    "types": ...,
    "import": ...,
    "require": ... // maybe CJS for this one for better compatibility :shrug: 
  }
}

@axelboc
Copy link
Contributor Author

axelboc commented Dec 4, 2023

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. import h5wasm from 'h5wasm/web or something of the sort.

@axelboc axelboc force-pushed the fix-exports branch 2 times, most recently from 3e44854 to bf89b2e Compare December 4, 2023 15:21
"type": "module",
"main": "./dist/node/hdf5_hl.js",
"main": "./dist/iife/hdf5_hl.js",
Copy link
Contributor Author

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/h5wasm@0.4.9');

package.json Show resolved Hide resolved
@bmaranville
Copy link
Member

bmaranville commented Dec 4, 2023

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!

@bmaranville
Copy link
Member

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.
h5wasm-0.6.11.tgz.zip

@axelboc
Copy link
Contributor Author

axelboc commented Dec 5, 2023

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:

No worries, I had managed to install everything, but then was running into permission issues when running make. I've reinstalled everything in a micromamba environment and it solved my problem!

image

  • The "Resolution failed" error is expected because Node 10 doesn't support exports
  • The "ESM (dynamic import only)" errors are expected since we don't provide a CJS bundle.
  • The "Internal resolution" errors hint at a problem with the way the types are being declared/imported in the code and how they are compiled with tsc, but this does not affect the issue at hand so I will try to fix it separately.

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. 🎉

@bmaranville bmaranville merged commit 34f69c6 into usnistgov:main Dec 5, 2023
1 check passed
@axelboc axelboc deleted the fix-exports branch December 18, 2023 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants