package.json exports resolution uses fallback conditions, unlike Node

[andrewbranch]

Bug Report

🔎 Search Terms

I’ve never been more confident, without searching, that I’m the first person to notice this

💻 Code

{
  "exports": {
    ".": {
      "import": "./dist/main.mjs",
      "types": "./dist/foo.d.ts"
    }
  }
}

🙁 Actual behavior

Importing this package from an ESM file, in --moduleResolution nodenext, searches for types at these locations:

• ./dist/main.mts
• ./dist/main.d.mts
• ./dist/foo.d.ts

🙂 Expected behavior

It should not search the types condition, because import already matched, and contained a valid target ./dist/main.mjs. Resolution here should fail for consistency with Node, which would throw a resolution error if it matched a condition but then failed to find the file specified in it.

Fixing this bug may cause more harm than good. I noticed it because I claimed that a popular library would be broken under certain conditions due to misconfigured exports, but was proven wrong in reality. I figured this may be worth documenting, but likely not worth fixing. But if someone can make a good case for why it matters, I would reconsider.

[frehner]

Thanks for reporting this! I think I ran into this while testing the types for a library I had created, but honestly thought it was more something I was doing wrong than something TS would be doing wrong.

Here's an additional library I've found that has types at the end instead of at the start:

headless-ui/react and headless-ui/vue

There's probably more out there as well.

[frehner]

As far as whether to fix it or not:

The pro of fixing it now is that it prevents the ecosystem from continuing this behavior, and thus allowing it to be entrenched.

Maybe at the least there could be a warning saying "this works now, but it's incorrect and may not work in the future" or something? 🤷

[andrewbranch]

Maybe at the least there could be a warning saying "this works now, but it's incorrect and may not work in the future" or something?

We don’t have a way to do warnings on the CLI, but #46861 is related.

[Andarist]

package.json

{
  "exports": {
    ".": {
      "require": "./dist/main.js",
      "types": "./dist/foo.d.ts"
    }
  }
}

pkg layout

dist/main.js
dist/main.d.ts
dist/foo.d.ts
package.json

Which .d.ts should be loaded? Should it be dist/main.d.ts or dist/foo.d.ts? Is a sibling .d.ts lookup still a thing when dealing with exports?

[andrewbranch]

Yes, sibling .d.ts lookup is always a thing, and it’s always the way I recommend people to structure packages when possible. In your example, main.d.ts would be looked up if the import was CJS-format; foo.d.ts would be looked up if the import was ESM-format.

[Andarist]

Yes, sibling .d.ts lookup is always a thing, and it’s always the way I recommend people to structure packages when possible.

Interesting. What's the argument for that? I usually assume that an explicit "types" might be better as it's more discoverable without the additional FS check/crawl (for instance npm is labeling packages with the TS label, I didn't check how they do it exactly but I wouldn't be surprised if they would only check the package.json content).

As to the original issue, IMHO this should be fixed on a correctness basis - to match the node's algorithm. Other tools are implementing their algorithm so having this bug~ here might cause people hard-to-debug issues when types will be found by TS itself but won't be found by other tools.

[andrewbranch]

I don’t mind if people want to explicitly list their types, but I dislike separating all the types into a different folder because it’s just so easy to mess up in subtle ways. Putting your .d.ts files next to your .js files is foolproof. Unfortunately it seems library authors by and large dislike this for some reason, and it causes all kinds of problems. So I recommend putting .d.ts files with their partners, for the sake of simplicity and explainability, and then if you want to add (strictly unnecessary) types keys to your package.json too, go for it.

[frehner]

One reason that library authors do it is because it is very easy to create conflicts between esm and cjs files and their types. I kind of alluded to that in the comment here, and especially since, as you mentioned above, the types for esm and cjs aren't always interchangeable.

Another pro of separating them out, though, is that the build output can also include other types of bundles - a "production" and "development" bundle, for example, where the types are the exact same but internally there's additional logging or other tools. Creating a matrix of esm | cjs | umd with development | production bundle outputs, all with the same types, just seems nicer to have a single types output folder to reduce the number of types that need to be generated, and prevent conflicts between them.

[Andarist]

Yeah, that's the main use case that comes to my mind. It's somewhat easy~ to create a sibling file for your package.json#main but once you start adding more "conditions" etc then we usually need a way to redirect them to that single set of .d.ts files (where other conditions in the past were just not observed by TS, so it wasn't as much of an issue).

But when parallelizing bundling and type generation, I would usually be wary of writing to the same directory at once. Writing to separate ones and using package.json#types seems to be a safer/more straightforward way.

But anyway... this all was quite offtopic. Thank you for sharing your thoughts though!

[hayes]

Here is another repro, was just trying to debug this for pothos

https://github.com/hayes/ts-esm-cjs-nodenext

So far the only workaround I have found is creating separate definition files for each build directory. Currently we publish a lib directory with cjs, an esm directory with esm, and a dts directory with definitions. To make this work, I would have to remove "types" from pacakge.json, and publish definitions in both the lib and esm directories, as well as continuing to publish in dts for backwards compatability. Would be awesome if there was a better option that didn't require publishing 3 identical definition files for every source file

[frehner]

Here is another repro, was just trying to debug this for pothos

https://github.com/hayes/ts-esm-cjs-nodenext

So far the only workaround I have found is creating separate definition files for each build directory. Currently we publish a lib directory with cjs, an esm directory with esm, and a dts directory with definitions. To make this work, I would have to remove "types" from pacakge.json, and publish definitions in both the lib and esm directories, as well as continuing to publish in dts for backwards compatability. Would be awesome if there was a better option that didn't require publishing 3 identical definition files for every source file

If it helps, I hacked it so that my build step makes a copy of my index.d.ts, renames the copy to index.d.cts, and then my package exports for "require"'s "types" points to that file. Everything works, and the only extra copy I have is a index.d.cts. 😬

I'm sure there's probably going to be issues with it, but it seems better than the alternatives at the moment.

[Andarist]

@hayes I was just working around this exact issue earlier today. You can check out what we have settled on at the end in this PR: https://github.com/alexreardon/tiny-invariant/pull/150/files

With the types condition being used first it's implied that this package is treated as a CJS package (it has no package.json#type set). And importing default export from a CJS package doesn't work in node - it returns the namespace object instead of the default export.

[andrewbranch]

That’s correct; https://github.com/hayes/ts-esm-cjs-nodenext is unrelated to this issue.

[Andarist]

This bug has actually cost me some debugging time yesterday when working on some tooling. I'm not sure if I would classify it with "Fixing this bug may cause more harm than good.". There is a clearly defined expected behavior for this and diverging from it might cause somebody to ship code with incorrect assumptions based on the observed broken behavior.

We tend to groan when we consider subtle differences between bundlers, wishing that they would behave in the same way. This issue here is effectively the same as a "subtle difference between tools" and one that could be avoided since, as far as I can tell, it wasn't really introduced on purpose.

[andrewbranch]

Fixing this would break 76 of the 2212 most popular packages on npm that ship their own types.

[Andarist]

That's 3% (not nothing but also not a substantial amount, i think) of packages that were misconfigured (as in, it truly works for them by accident - which I feel is the fair assessment here since you created this issue as a bug).

Imagine that I'm implementing a package validator - like yours arethetypeswrong or publint. Should this be an issue raised by it? If yes... why, it's working fine according to TS implementation. If no... oh, I need to add code specific to TS to handle this.

A potential for a similar situation could be found in a "type bundler" tool. It should know how to correctly load types but it might have a hard time achieving 100% correctness if TS diverges from node here.

I see package.json#exports as an interoperability tool~. While it's extensible by nature (conditions ftw), it still has some core concept backed into it. This might sounds like a small issue but IMHO this only expands the (in)compatibility table that gives us migranes already. I'd much prefer to keep it smaller instead of adding columns to it 😬

At the end of the day, I think that's up to you to decide what's right here. I personally think though that it's important to establish what would be the desired behavior as if you'd be implementing this today. I just can't get behind "it's a bug but we tolerate it" myself - if it's a bug then it can/should be fixed in my mind.

[andrewbranch]

The typical case here is that packages have a fundamental flaw of containing only CJS types for a dual ESM/CJS package, because those packages are depending on a build tool that produced, in the best case scenario, a pair of type declarations and JS files in one output format which were validated to be correct by TypeScript, and one set of completely unsafe JS files without types in the other output format. Then on top of that, they make a package.json mistake which, through this bug, causes TypeScript to find the one set of types that exists, when technically it should have found nothing. The experience for the end user consuming these types is that they can probably get by, instead of being totally blocked. And while the package.json ordering error is easy to correct, fixing it still leaves the package objectively misconfigured. On the other hand, if the fundamental problem of missing types is solved, then the occurrences of this bug completely vanish, because the earlier matching lookup condition doesn’t fail to resolve. But as you know, that problem is much harder to solve. It feels really bad to break 76 super popular packages for millions of users and then tell them, when they inevitably file bugs against TypeScript, that the packages are misconfigured and they have to completely ditch their whole build system, and maybe they should rethink shipping a dual ESM/CJS package entirely, just to get types—when they had probably-working types in the last version of TypeScript. People would think we’ve lost our minds.

(as in, it truly works for them by accident

It’s certainly not an accident for the package authors; through TypeScript’s accident, the package authors are getting the closest thing to their intended behavior that’s possible with missing types.

Imagine that I'm implementing a package validator - like yours arethetypeswrong or publint.
Should this be an issue raised by it?

Look, I also hate this. I want things to be correct. You will rarely see me argue that wrong module resolution is good. But shipping a break for this is not the hill I want to die on. I think we could consider it for 6.0. Alternatively, if we see the misconfiguration essentially eradicated from the observable npm universe before then, maybe it will approachable.

Feel free to get started 😄

[
  'tslib',
  'yargs',
  'socket.io',
  'acorn',
  'eventemitter3',
  'socket.io-client',
  'sequelize',
  'colorette',
  'concurrently',
  'react-select',
  'isomorphic-unfetch',
  'regexpp',
  'csv',
  'is-promise',
  'zod',
  'react-inspector',
  'listr2',
  '@storybook/react',
  'cypress',
  '@storybook/addons',
  'acorn-walk',
  'cjs-module-lexer',
  '@storybook/theming',
  'react-toastify',
  '@storybook/csf',
  '@storybook/core-events',
  '@storybook/api',
  'geolib',
  'dexie',
  '@storybook/client-logger',
  'discord-api-types',
  'ngx-bootstrap',
  '@storybook/addon-actions',
  'sqlite',
  'vue-demi',
  'markdown-to-jsx',
  'deep-object-diff',
  '@storybook/channels',
  '@fortawesome/fontawesome-svg-core',
  '@fortawesome/free-solid-svg-icons',
  'pathe',
  'hls.js',
  '@storybook/store',
  '@storybook/router',
  'react-merge-refs',
  '@storybook/core-common',
  '@storybook/docs-tools',
  '@headlessui/react',
  '@storybook/preview-web',
  '@discordjs/builders',
  'preact-render-to-string',
  'lmdb',
  '@storybook/cli',
  '@storybook/addon-essentials',
  'is-what',
  '@storybook/channel-websocket',
  'favicons',
  '@storybook/builder-webpack5',
  'async-mutex',
  'copy-anything',
  '@storybook/telemetry',
  'amqp-connection-manager',
  '@discordjs/rest',
  'parse-duration',
  '@storybook/components',
  '@storybook/addon-outline',
  '@storybook/addon-interactions',
  '@storybook/addon-measure',
  '@storybook/addon-links',
  'react-idle-timer',
  '@discordjs/collection',
  '@solana/spl-token',
  '@storybook/addon-toolbars',
  'msgpackr',
  '@storybook/addon-docs',
  '@urql/core'
]

[andrewbranch]

The detectable occurrence of this problem in the top ~5k npm packages is down to 14 packages:

[
  'yargs',
  'acorn',
  'highlight.js',
  'sequelize',
  'colorette',
  'concurrently',
  'isomorphic-unfetch',
  'regexpp',
  'react-inspector',
  'is-promise',
  'acorn-walk',
  'cjs-module-lexer',
  'zod',
  '@storybook/csf'
]

Impressive work, @Andarist

[Andarist]

Some of the listed ones landed the fixes for this. I’ll take around across them to recheck their release statuses and gonna get back here with a 1 by 1 breakdown

[andrewbranch]

Ooooh, my bad 😬 There was a bug in my script and that’s not the whole list. There are actually 29:

[
  'yargs',
  'acorn',
  'sequelize',
  'highlight.js',
  'colorette',
  'concurrently',
  'isomorphic-unfetch',
  'is-promise',
  'zod',
  'regexpp',
  'react-inspector',
  'acorn-walk',
  'cjs-module-lexer',
  '@storybook/csf',
  'geolib',
  '@fortawesome/fontawesome-svg-core',
  'ngx-bootstrap',
  'markdown-to-jsx',
  'deep-object-diff',
  '@fortawesome/free-solid-svg-icons',
  'pathe',
  '@headlessui/react',
  'async-mutex',
  'parse-duration',
  'favicons',
  'antlr4',
  'check-disk-space',
  'react-idle-timer',
  '@solana/spl-token'
]

Still amazing progress! 😅

[Andarist]

package	status
yargs	approved but not merged yargs/yargs#2327
acorn	merged but not released
sequelize	merged but not released but the fix got merged into their v7 alpha that also renames the package, prepared a new PR that targets the current latest release: sequelize/sequelize#16085
highlight.js	this wasn't on the original list, the issue itself was merged on Mar 19: highlightjs/highlight.js#3736 , prepared a fix for it: highlightjs/highlight.js#3800
colorette	merged but not released
concurrently	merged but not released
isomorphic-unfetch	no response in the PR, it's developit's (Jason Miller, Preact's author) package. He is busy and didn't see the PR - it's likely that we could get his attention on this using other channels
is-promise	no response in the PR
zod	merged but not released
regexpp	no response in the PR, it's likely that this won't be addressed. There is a community fork of this package (@eslint-community/regexpp) and the issue is fixed there.
react-inspector	merged but not released
acorn-walk	merged but not released
cjs-module-lexer	merged but not released
@storybook/csf	I missed this initially because it's a @storybook/* package but yet it's not in their monorepo. Prepared PRs to fix this right now
geolib	merged but not released
@fortawesome/fontawesome-svg-core	2 weeks ago I got an info that the issue was fixed (here) and that it should go out in the next release. There was no new release yet though.
@fortawesome/free-solid-svg-icons	same as above
ngx-bootstrap	I couldn't fix this easily because it's an Angular project and it's using a lot of indirection to generate their packages. I created an issue about this but didn't get any response so far. I think that's actually an issue with the lib builder that they are using (ng-packagr) but couldn't yet find the solution for it. Note that this package is just plainly broken - package.json#exports.types are pointing to files that don't exist. The intention was clearly for them to exist but for some reason those files are not generated. So it's a pure luck that it manages to fall back to types through other lookup mechanisms
markdown-to-js	merged but not released
deep-object-diff	no response in the PR
pathe	merged but not released
@headlessui/react	merged but not released
async-mutex	no response in the PR
parse-duration	merged but not released
favicons	merged but not released
antlr4	this wasn't on the original list, the issue was created on Apr 13: antlr/antlr4@a9e6978 , prepared a fix for it: antlr/antlr4#4297
check-disk-space	this wasn't on the original list, the issue was created on Apr 20: Alex-D/check-disk-space@2a95bf8 , prepared a fix for it: Alex-D/check-disk-space#28
react-idle-timer	soft-approved but not merged SupremeTechnopriest/react-idle-timer#343
@solana/spl-token	merged but not released