-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
package.json exports
resolution uses fallback conditions, unlike Node
#50762
Comments
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 headless-ui/react and headless-ui/vue There's probably more out there as well. |
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? 🤷 |
We don’t have a way to do warnings on the CLI, but #46861 is related. |
package.json {
"exports": {
".": {
"require": "./dist/main.js",
"types": "./dist/foo.d.ts"
}
}
} pkg layout
Which |
Yes, sibling |
Interesting. What's the argument for that? I usually assume that an explicit 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. |
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) |
One reason that library authors do it is because it is very easy to create conflicts between 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 |
Yeah, that's the main use case that comes to my mind. It's somewhat easy~ to create a sibling file for your 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 But anyway... this all was quite offtopic. Thank you for sharing your thoughts though! |
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 |
If it helps, I hacked it so that my build step makes a copy of my I'm sure there's probably going to be issues with it, but it seems better than the alternatives at the moment. |
@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 |
That’s correct; https://github.com/hayes/ts-esm-cjs-nodenext is unrelated to this issue. |
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. |
Fixing this would break 76 of the 2212 most popular packages on npm that ship their own types. |
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 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 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. |
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.
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.
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'
] |
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 |
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! 😅 |
|
Bug Report
🔎 Search Terms
I’ve never been more confident, without searching, that I’m the first person to notice this
💻 Code
🙁 Actual behavior
Importing this package from an ESM file, in
--moduleResolution nodenext
, searches for types at these locations:🙂 Expected behavior
It should not search the
types
condition, becauseimport
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.The text was updated successfully, but these errors were encountered: