Support Node.js ESM

[jaydenseric]

Fixes #640 .

[jaydenseric]

A slightly different approach would be to add a package.json exports field to allow extensionless conditional CJS/ESM deep imports from styled-jsx/css, but there is a lot to consider about adding an exports field and it would a breaking change. This PR is non-breaking.

[herodrigues]

@jaydenseric @giuseppeg any progress on this?

I've seen many issues from people building npm modules that use styled-jsx, like this one:
https://stackoverflow.com/questions/55050039/intense-fouc-when-using-next-js-8-and-styled-jsx

[giuseppeg]

having to include the .js extension in the imports is a breaking change

[jaydenseric]

@giuseppeg what do you mean. I already said:

This PR is non-breaking.

The imports documented in the readme right now are invalid for Node.js, they just happen to currently work if you have Webpack etc. setup in your project. Nothing is stopping users from importing the old way; we should document the right way though.

[giuseppeg]

@jaydenseric sorry I meant that by default the docs should mention import css from 'styled-jsx/css' instead of import css from 'styled-jsx/css.js'. This is browser code (except for styled-jsx/server) not Node.js so perhaps the exports map way is the most appropriate? We could transpile the code for now to not make it a breaking change and later fine tuning the transpilation settings.

[giuseppeg]

@jaydenseric sorry I updated the comment, would that work?

[jaydenseric]

There is a lot to unpack in your comment.

This is browser code

Not true, React components are often rendered in a Node.js environment.

All Node.js/npm ecosystem tooling should adopt the final Node.js ESM paradigm. For example, there are ESLint plugins that rightfully report that import css from 'styled-jsx/css' doesn't resolve anything.

Regardless, even if that code only ran in a browser there is no harm in specifying file extensions; that is actually more browser friendly not less.

perhaps the exports map way is the most appropriate

If you are referring to import maps, they don't relate to Node.js ESM support.

If you are referring to adding a package.json exports field with conditional ESM/CJS entrypoints, they yes that is a great idea. It is however a breaking change to add a package.json exports field. That is how I do things in my own packages:

https://github.com/jaydenseric/extract-files/blob/1c1d3e4611616492333e13db166c7f4bb0c5fca5/package.json#L32-L40

It is also the recommended way to publish packages that support being imported from ESM and required from CJS whilst avoiding the dual package hazard.

Individual parts of the API should be published as CJS available via deep imports, and the ESM index should pull in the same CJS (don't publish the same code in multiple builds/formats; that causes the dual package hazard) allowing it to be accessed via ESM named imports. E.g:

https://github.com/jaydenseric/extract-files#function-extractfiles

We could transpile the code for now to not make it a breaking change and later fine tuning the transpilation settings.

Sorry I don't understand what you mean by this, how how it relates to anything being discussed.

[giuseppeg]

I agree I just think that

even if that code only ran in a browser there is no harm in specifying file extensions; that is actually more browser friendly not less

is not wide spread (not saying that it is a good reason).

If you are referring to import maps, they don't relate to Node.js ESM support.

I was thinking about https://nodejs.org/api/esm.html#esm_conditional_exports i.e. the alternative solution you proposed. How is that a breaking change? Can we default to the current behavior?

I should do some research in order to understand the benefits/tradeoffs and make a call.

[jaydenseric]

Adding an exports field to package.json with conditional exports is a breaking change because:

1. Early versions of Node.js that don't support the exports field ignore it and fall back to the main field, except for early v13 versions that support basic exports but not yet conditional syntax which was unflagged in v13.7.0. This means the package engines field needs to be updated to exclude these early v13 versions, e.g:

   https://github.com/jaydenseric/extract-files/blob/1c1d3e4611616492333e13db166c7f4bb0c5fca5/package.json#L42

   This breaking change is generally acceptable because v13 was not LTS and is EOL.

2. Anything not explicitly exported becomes private, so any deep imports people were making to files no longer public will break. This is actually a good thing; it allows you to very clearly control what is private and public - I like to even put the code in directories called private and public, e.g:

   https://unpkg.com/browse/graphql-upload@11.0.0/

   Note that if you define an export with or without a file extension, that becomes the only way you can import or require the file, unless you define the export both ways. I find it a lot easier to export the entire public directory instead of attempting to export individual files, e.g:

   https://github.com/jaydenseric/extract-files/blob/1c1d3e4611616492333e13db166c7f4bb0c5fca5/package.json#L37

3. This breaking change can be avoided, but most people forget about this: You need to explicitly export the package.json file itself, both with and without the file extension because people require it with and without the extension surprisingly often for all sorts of exotic reasons. E.g:

   https://github.com/jaydenseric/extract-files/blob/1c1d3e4611616492333e13db166c7f4bb0c5fca5/package.json#L38-L39

Often a package supports the same environments for all the published files (e.g. extract-files), but sometimes a package has environment specific exports (e.g. universal and server only, like graphql-react). I think styled-jsx is an example of the latter? Anyway, in that case it is nice to have private folders nested inside the universal and server folders so that they can receive appropriate transpilation according to the directory scoped .babelrc files. They can be blacklisted from being exported using {}, e.g:

https://github.com/jaydenseric/graphql-react/blob/4de3182051afa78400657212ef009624a91b7efb/package.json#L45

Note that only recent Node.js versions support blacklisting exports via null; but by luck all versions supporting exports support {}. For the backstory on this trick (and how null was added to the Node.js implementation), see jkrems/proposal-pkg-exports#46 .

[huozhi]

Hi @jaydenseric , we're trying to ship a new version with few breaking changes. I feel this is good to include the ESM support there.

styled-jsx plugins loading need to be supported with ESM if this is going to landed. Would you support that one as well in the PR and resolve the conflicts, and targeting to alpha branch (in this temporary period) then we can land it after everything looks good! 🙏

[jaydenseric]

@huozhi it's great to see some movement towards proper ESM support, thanks for your recent maintenance work and for giving me the opportunity to see this PR through.

This PR was mergeable over a year ago when I originally put careful effort into this contribution, and really needed it. Since then I had to migrate away from styled-jsx and as such am no longer motivated to work on these problems for Vercel Inc. pro-bono. You should have push rights to this PR branch though so feel free to take it from here; I just changed the target branch from master to alpha for you.