-
-
Notifications
You must be signed in to change notification settings - Fork 125
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
fix: add types condition to the front of the exports #155
Conversation
This is a "correct" fix to get rid of the "🐛 Used fallback condition". There are other issues with this particular // @moduleResolution: node16
// @module: commonjs
// The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("ts-pattern")' call instead.
// To convert this file to an ECMAScript module, create a local package.json file with `{ "type": "module" }`.(1479)
import { match } from 'ts-pattern' // types: 4.2.2 |
Thanks for the PR @frankie303, and for pointing out this additional issue @Andarist. I tried fixing most problems surfaced by "Are the types wrong?" and here is what I've got so far:
I wasn't able to find a way to fix the |
It's not exactly that it thinks that mistakenly - it has some good reasons to think that. If you provide different implementations (through import+require conditions) then it only makes sense to provide different types as well (even if their content is pretty much the same). Consider for example this case: const { SomeClass } = require('lib')
const instance = new SomeClass()
console.log('is it the same?', instance instanceof (await import('lib')).SomeClass) This logs false because you have different ESM/CJS implementations and TypeScript has to account for such things. This particular example can impact the control flow analysis and thus type narrowing etc. |
A potential fix for this is:
|
Thanks @Andarist, that makes sense. The only issue is that I'm not bundling my declarations into a single
|
I'm going to merge #158 as it's already an improvement. Tested it with a few project configurations and it seems to work well |
I certainly wasn't implying that you should copy-paste this stuff by hand :p I imagine that a simple script could do that for you and that you could just add it as a step to your build/prepare scripts. Smth like this could work: find dist -name '*.d.ts' | while read NAME; do mv $NAME ${NAME%.d.ts}.d.cts; done Note that it's also not a bullet-proof solution (see Andrew's comment here) but most of the time... it's good enough. Another "good enough" solution (although, IMHO, way less correct than copy-pasting declaration files) is to remove
I didn't yet see any of the lib builders to actually do that. I can recommend using https://github.com/preconstruct/preconstruct/ . I might be biased here (since I'm sort of a partial maintainer of that project) but we put a lot of effort into making it as seamless as possible for variety of use cases and edge cases. Note that, for important reasons, we do not emit dual packages there. |
Well, I believe you would also need to change all imports of all module to include the Thanks for these explainations though. I think I'm just going to leave the |
Ah, ye - right, good catch. This is avoidable if you nest your CJS files in a directory with an extra { "type": "commonjs" }
For what it's worth - I ran into this problem in the past, just with different packages than |
I think I finally have got something that works well enough in PR #160 https://arethetypeswrong.github.io/?p=ts-pattern%404.2.4-test.0 I went with some files=$(find dist -type f -name "*.d.ts");
# Loop through the declaration files
for file in $files; do
# Update imports to include the '.cjs' extension
sed -E "s/(.*)from '([^']*)'/\1from '\2.cjs'/g" "$file" > "${file%.d.ts}.d.cts"
done And I updated the "exports": {
".": {
"require": {
"types": "./dist/index.d.cts",
"default": "./dist/index.cjs"
},
"import": {
"types": "./dist/index.d.ts",
"default": "./dist/index.mjs"
},
"types": "./dist/index.d.ts",
"default": "./dist/index.mjs"
},
"./package.json": "./package.json"
}, I'm by no means a bash expert, so please let me know if you catch problems with this code :) |
Yeah, I'm not a bash expert either 😅 I would just rewrite this into a JS script for readability and maintainability. Despite passing all checks on arethetypeswrong - this isn't currently consumable from ESM:
To observe this you might want to disable I think that this should be reported by To fix this you can just include explicit extensions in both of those declaration "sets". It's not that Small nit: it probably doesn't create any problems in this setup but using non-matching extensions between implementation and declaration files really bugs me. It would be best to use either |
Thanks, I added explicit See https://arethetypeswrong.github.io/?p=ts-pattern%404.2.4-test.1 Do you know if explicit extensions in |
I can't say that I'm like 100% sure - but I think they are supported just fine. You can verify here that it works even with 4.1: TS playground (I didn't bother testing other versions :P) |
I coincidentally saw that there is an issue with TypeScript which could cause some potential bugs in libraries. I also saw that @Andarist was opening some pr-s (example) in popular libraries to fix the bug. So I just moved types to the top in exports.
So I think this pr should fix
🐛 Used fallback condition
issues here.