ESM build is not compatible with Node ESM loader

[e1himself]

Describe the bug

The @restart/ui package provides ESM and CommonJS builds. However, the ESM build is not really compliant with Nodejs ESM module resolution.

To Reproduce

1. Start a new Nodejs package with type: "module"
2. Add @restart/ui as a dependency
3. Add index.js file containing the following:

   import { useRootClose } from '@restart/ui';
   console.log(useRootClose);

4. Run the file with node index.js

Reproducible Example

https://github.com/e1himself/restart-ui-esm-incompatibility-demo

The test run:
https://github.com/e1himself/restart-ui-esm-incompatibility-demo/actions/runs/4915733576/jobs/8778577614

Expected behavior

The ESM module import successfully resolves.

Actual behavior

import { useRootClose } from '@restart/ui';
         ^^^^^^^^^^^^
SyntaxError: Named export 'useRootClose' not found. The requested module '@restart/ui' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '@restart/ui';
const { useRootClose } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:123:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:189:5)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:530:24)
    at async loadESM (node:internal/process/esm_loader:91:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)

Environment (please complete the following information)

• Operating System: Fedora Linux
• Node v16.20.0
• NPM 8.19.4

[jquense]

the error seems correct here? this is a common js package on the server, so you need to import it as a single default export.

restart/ui doesn't provide "real" esm builds that node can injest, just ones bundlers can, which are less strict in ways that make providing something like an esm build possible. Full node esm compatibility is still not worthwhile and way more trouble than it's worth.

[e1himself]

the error seems correct here? this is a common js package on the server, so you need to import it as a single default export.

Yes, it's correct. But the reason is different.

If it was a purely common js package, it could be working, even with an ESM import.

But this package pretends to be an ESM-ready package by declaring conditional exports, which does not really contain an ESM-ready code. That's why it fails to import.

[e1himself]

I totally see your point that making this 100% compliant with ESM syntax might not seem worth the effort, but I respectfully disagree.

1. The package uses Node.js conditional exports feature, which is designed to provide different (CommonJS, ESM, other) entry points to a package.
2. The problem with a "halfway" ESM implementation that bundlers can ingest is that there is no formal way of validating it. I've already encountered half a dozen of misconfigured popular JS libraries that cannot be really used in ESM-preferring environments (even bundlers). On the other hand, a proper ESM implementation can be validated by running it in node.

Long story short: I honestly believe that declaring ESM compatibility, but not providing it, will do the ecosystem a bad favour in the long run.

[jquense]

Sure, that is sort of annoying but the fix is the same here i think? switch to a require()?

@kyletsang I guess we should figure out if there is a way to do exports in package.json for bundlers but not node?

Long story short: I honestly believe that declaring ESM compatibility, but not providing it, will do the ecosystem a bad favour in the long run.

I don't disagree with this the issue is that it's basically impossible for us to make the package "real" esm that works everywhere. Without diving into specifics the crux of the issue is that because of other faux esm packages we can't be a real package either, due to how types are declared. You end up in situations where it's literally impossible to import a dependency in a way that will satisfy TS, bundlers and node. I absolutely wish this wasn't the case, but every time i've attempted to just use real modules for both browsers, bundlers, and node it doesn't work.

We aren't really trying to "declare esm compatibility" so much as make the bundler experience better. e.g. the ESM is for browsers/bundlers and commonjs for node

[e1himself]

I guess we should figure out if there is a way to do exports in package.json for bundlers but not node?

I believe providing only main and module properties is the way. AFAIK Node.js doesn't rely on module in any way, but bundlers do.

Though, as a side effect, this will drop the ability of deep imports. Is this a desired feature?

[jquense]

Yeah deep imports are required

[jquense]

I don't know how precedence works exactly for the exports conditions but i guess we could use node: to point to the CJS build here. let me try that

[e1himself]

A proper fix isn't that difficult. Please see this PR for react-bootstrap/dom-helpers: react-bootstrap/dom-helpers#180

[jquense]

Adding file extensions isn't sufficient here for the reasons noted above, unless you are going to do the same for every dependency here?

Ultimately tho there is still no benefit to anyone if we did that. What do we get from having node esm support? There are really only downsides right now, it limits which dependencies we can depend on without extra work, makes the build more complicated or limits consumer usage. That would be fine if we got anything out of it but at the moment I don't see any advantage vs just providing a cjs build?

[jquense]

And to be clear I'm not against this, but having just tried to do this with another package and failing I'm reticent to try again right now without a clear benefit and goal beyond just supporting esm in node.

[jquense]

And to be clear I'm not against this, but having just tried to do this with another package and failing I'm reticent to try again right now without a clear benefit and goal beyond just supporting esm in node.

[e1himself]

... unless you are going to do the same for every dependency here?

Yeah, that's correct. But we have to start somewhere, and hopefully, the community will catch up sooner than later. Where do we start if not with our packages? :)

Also, it's not that bad. Many modern packages already do this the proper way. And for older non-ESM packages, importing CommonJS code also mostly works.

Ultimately tho there is still no benefit to anyone if we did that. What do we get from having node esm support?

But it's not just Node ESM support, that's the general ESM loader spec, defined for all environments. Browsers implement it too.

The benefit of validating it against Node (as a reference implementation) is that we can be 100% sure that the exported ESM code is compliant with the spec. The bundlers build on top of the ESM loader spec the same way Node does, adding more permissive behaviour on top of the spec. But being strictly compliant with the spec will guarantee it will work with all modern bundlers.

[jquense]

importing CommonJS code also mostly works.

The issue is that this isn't actually straightforward. I just encountered this elsewhere. If you try and import a module as CJS but the types say it's a module it will result in type errors and invalid output. For bundlers like webpack, vite, rollup, etc they will complain that the code is importing the dependency incorrectly but it will work in node. Alternatively it won't in node but ts/bundlers don't complain. This can be worked around with conditional exports and imports but that adds complexity for no payoff. The benefit of esm is that i can have a single build that works in both places, but if we are still going to end up with multiple builds per target, it's preferrable (imo) to stick with common js for those cases since there is no real downside to using CJS in node (but upside to using even faux modules in bundlers).

being strictly compliant with the spec will guarantee it will work with all modern bundlers.

see above!

Honestly i would like to do this, but i don't really have the time to dedicate to working out the kinks, especially if the exploration results in a bunch of work updating and PRing deps only to have a more complex setup on the other end. If you want to take a stab at it happy to help test a bit.