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

feat: for platforms supporting conditional exports, when importing tiny-invariant from an ESM module, serve an ESM build #145

Merged
merged 2 commits into from
Sep 28, 2022

Conversation

pkerschbaum
Copy link
Contributor

@pkerschbaum pkerschbaum commented Sep 16, 2022

Fixes #144

Current Situation

When tiny-invariant is imported from an ESM module in Node.js, like this:

import invariant from 'tiny-invariant';

...the CJS build of tiny-invariant is served.

Here's why 👇
  • Node.js will look up tiny-invariant in node_modules and look into package.json.
  • It first looks for the property exports, but since this is not present in the currently published version of tiny-invariant, it looks at the property main. This property refers to "dist/tiny-invariant.cjs.js". From this point on it is clear that this file will get executed.
  • What is unknown at this point, however, is whether the file contains CJS or ESM code. The file extension .js is ambiguous, it can contain either CJS or ESM code. To know in which module mode Node.js should run this file, it will search for the "closest" package.json file and look at the type property. There is no package.json in the folder dist, so it looks one level up, finding package.json of tiny-invariant itself. type is not set, so Node.js interprets the file as a CJS file.

Goal of this Pull Request

This pull requests adds an ESM build which is used by Node.js and any other platform respecting "conditional exports" when importing tiny-invariant from an ESM module.
This fixes #144.

It uses the technique outlined by 2ality.com/2019/10/hybrid-npm-packages.html#variant%3A-avoiding-.mjs, with the only difference that tiny-invariant is still CJS by default and ESM by opt-in.
The same technique is used by safe-stable-stringify.

Risks

This should be almost backwards-compatible, because:

  • platforms not supporting the "exports" field will still work as before,
  • and those platforms respecting the "exports" field will use the new file only if an ESM import is used to consume the package. And then, it will get the same JS as it would have before when it consumed dist/tiny-invariant.esm.js.
There's one catch though I am aware of: Reaching into `dist` might not be possible anymore 👇

exports is now defined as:

"exports": {
  ".": {
    "import": "./dist/esm/tiny-invariant.js",
    "default": "./dist/tiny-invariant.cjs.js"
  }
},

Once the property exports is present in package.json, platforms respecting that property will refuse to import anything not specified in exports.
Thus if some codebase reaches into dist like this (and the platform respects exports):

import invariant from 'tiny-invariant/dist/tiny-invariant.min.js';

...this will not work anymore.

We could adapt exports such that everything from dist is exported, but I wonder if we should do that.
This potential problem can affect only modern codebases respecting exports, and I would expect that almost everyone is importing tiny-invariant with the "bare module specifier":

import invariant from 'tiny-invariant';

Additional Notes

@alexreardon
Copy link
Owner

Thanks heaps! I will take a look at this first thing next week

package.json Outdated
@@ -54,8 +60,10 @@
"build:clean": "rimraf dist",
"build:flow": "cp src/tiny-invariant.js.flow dist/tiny-invariant.cjs.js.flow",
"build:typescript": "tsc ./src/tiny-invariant.ts --emitDeclarationOnly --declaration --outDir ./dist",
"build:typescript:node16": "tsc ./src/tiny-invariant.ts --emitDeclarationOnly --declaration --outDir ./dist/node16",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be called "node16" or "esm"? Soon we will have Node 18 as standard, and having node16 in the path might be confusing for folks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, I renamed "node16" to "esm" :)

@alexreardon
Copy link
Owner

I'll release this change as a beta npm release to test it out a bit

@@ -0,0 +1,3 @@
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm.. can you please explain what this file is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NPM script build:copy-esm-packagejson puts this file into the folder ./dist/esm.
It's sole purpose is that Node.js interprets ./dist/esm/tiny-invariant.js as an ESM module.

Dr. Axel Rauschmayer wrote a blog post about "Hybrid npm packages", and Option 1 is the one we want to enable ("ESM and CommonJS are both bare imports"): https://2ality.com/2019/10/hybrid-npm-packages.html#option-1-(experimental%2C-needs-conditional-exports)%3A-esm-and-commonjs-are-both-bare-imports.
Note that this blog post was last updated in 2019 and the things mentioned as experimental are not experimental anymore.

@alexreardon
Copy link
Owner

I am leaning towards merging this to get people unstuck. I am also thinking of doing a major soon where this project moves to esm only

@pkerschbaum
Copy link
Contributor Author

What is your concern about merging this?
I can explain the full problem and possible solutions again in great detail if you want to :)

I would probably stick with a CJS build because I think with the conditional export (exports keyword in package.json), both CJS and ESM are supported just fine

@alexreardon
Copy link
Owner

Hey sorry, been busy today. I plan on merging and releasing this soon. My hesitation has been due to my lack of familiarity with the details of this problem domain.

@pkerschbaum
Copy link
Contributor Author

Hey, no problem, for me it's not urgent.

@pkerschbaum pkerschbaum changed the title fix: create a separate build for node16 TypeScript module mode (#144) fix: serve an ESM build to ESM modules importing tiny-invariant (#144) Sep 21, 2022
@pkerschbaum pkerschbaum changed the title fix: serve an ESM build to ESM modules importing tiny-invariant (#144) fix: serve an ESM build to ESM modules importing tiny-invariant (#145) Sep 21, 2022
@pkerschbaum pkerschbaum changed the title fix: serve an ESM build to ESM modules importing tiny-invariant (#145) fix: serve an ESM build to ESM modules importing tiny-invariant Sep 21, 2022
@pkerschbaum pkerschbaum changed the title fix: serve an ESM build to ESM modules importing tiny-invariant fix: for platforms supporting conditional exports, when importing tiny-invariant from an ESM module, serve an ESM build Sep 21, 2022
@pkerschbaum
Copy link
Contributor Author

...I just did a big update of both the title of this PR and the description. @alexreardon please read it through once again, I discovered one risk (outlined in the description).

I am still in favor of merging this PR. I expect very little to no problems with existing codebases; it fixes #144; and an ESM build is then available to consumers of this package.

The alternative would be to not merge this PR and instead fixing #144 by modifying the types of this package. I actually tried that at the beginning but could not get it to work, the compilation problem would just stay.
There is some discussion in microsoft/TypeScript#50690 about different styles of typings, but I still don't know what should be done, and I think modifying dist/tiny-invariant.d.ts would impose even more risk to break existing codebases...

@alexreardon
Copy link
Owner

alexreardon commented Sep 25, 2022

I am now back at work so I can help see this through. Thank you for your thoroughness @pkerschbaum. I think reaching into dist is not public API so I think we are safe there. We could also release this as a major to be extra safe.

So right now I am thinking:

  • merge change
  • release a major so nobody will break

An alternative course

I have been thinking of moving all of my packages to esm only.

Moving to esm only should:

  • fix the current issue
  • still release a major
  • anybody who still needs non-esm can use the last major
  • drop some of the added complexity of this PR
  • drop build complexity as we would move to a single bundle; potentially just generated with tsc

What do you think @pkerschbaum ?

@theKashey
Copy link

👍 👍 👍 👍

  • there is no type:module in the root package.json, so the "public API" does not change. No major version change should be required.
  • I would personally propose pointing to "import": "./dist/esm" and then pointing to the specific file via package.json in the directory. 🤷‍♂️ Just to enforce the connection.
  • and, do not use default exports next time 😅

@pkerschbaum
Copy link
Contributor Author

@alexreardon

I have seen that ESM Readme of sindresorhus many times and I really don't like the "convert your own codebase to ESM or just stick with the old version" attitude.
I have had it many times that I updated a dependency only to break it completely because it moved to pure ESM. Sticking with the old version means no new features/no bug fixes/no security fixes, but converting a codebase to ESM can be a difficult and costly endeavour.
Try to explain to mgmt of an enterprise that you need to do a costly huge change of your codebase with potential for regressions for no real business-relevant benefits :)

However, in the case of tiny-invariant the "stick with the old version for CJS" approach is OK - there is very probably no need for new features over the lifetime of this package; it is 100% bug free; and it does not have any potential for security issues.
Thus, I am fine with both solutions 😁 I have a slight preference for this PR because it is the "nicest" for package consumers.
In the case of the ESM-only approach, I would add a hint at the beginning of the README that version 1.x must be used for CJS. Otherwise developers will have the journey I had a couple of times (adding a dependency - oh, there are compile/runtime errors - revisit the README and version history of the GitHub repo to discover that it is ESM only and which version to use to get CJS to work - add that version).

In case of the ESM-only approach, we could close this PR and I could create one which just outputs ESM using tsc and nothing else (no rollup etc).

You decide which direction we go 😄

@theKashey

there is no type:module in the root package.json, so the "public API" does not change. No major version change should be required.

That's true but I think @alexreardon just wants to ensure that no platform/bundler out there breaks because of this change (and because of them not having a proper lockfile...).

I would personally propose pointing to "import": "./dist/esm" and then pointing to the specific file via package.json in the directory. 🤷‍♂️ Just to enforce the connection.

This does not work (I just tried it to confirm that). Package entry points (the "exports" property of package.json) is taken into consideration when Node.js (or other platforms/bundlers) resolves a "bare module specifier". But never after that. See https://2ality.com/2022/01/esm-specifiers.html#resolving-module-specifiers-in-node.js and https://nodejs.org/api/packages.html#package-entry-points.

and, do not use default exports next time 😅

I agree with that, but I would stick with a default export of tiny-invariant even if we do a major version change because otherwise people would need to rewrite import statements ;)

@alexreardon
Copy link
Owner

Okay, I'll aim to merge and release tomorrow

@pkerschbaum pkerschbaum changed the title fix: for platforms supporting conditional exports, when importing tiny-invariant from an ESM module, serve an ESM build feat: for platforms supporting conditional exports, when importing tiny-invariant from an ESM module, serve an ESM build Sep 27, 2022
@alexreardon alexreardon merged commit 98970f6 into alexreardon:master Sep 28, 2022
@alexreardon
Copy link
Owner

@pkerschbaum @theKashey I have released this change in 1.3.0

https://github.com/alexreardon/tiny-invariant/releases/tag/v1.3.0

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.

Incorrect Types when using in a TypeScript ESM project
3 participants