-
-
Notifications
You must be signed in to change notification settings - Fork 500
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: Use rewritten cosmiconfig-typescript-loader package for handling TS config files. #347
Conversation
- Fixes issue where incompatible module types are loaded implicitly.
Before merging this PR, I would like to have your opinion @alvis. |
I've tested this on my own usage of craco, but it would be great to have some additional sanity tests done too 👍 |
Hi @Codex- Thanks for making However, there are few points I'm not sure adopting
|
It's worth noting that the current implementation introduces a dependency on Craco now also suffers from Pick your poison I suppose. The REPL for this build context should be fine, you're already registering the REPL anyway and the config used by default (with how tsconfig resolution is handled) in the case of I explored using ts-nodes REPL and the TS service but ended up in a scenario that was rapidly growing unruly lol. There's extensive discussion about multiple registrations of ts-node. |
Thanks for pointing out. It's true and it's a mistake should be fixed.
Though I'm not sure if overriding is the best solution, but I agree the current implementation requires module in tsconfig to be set to commonjs, or require users to use a separate tsconfig.json with commonjs module for building. For these reason, I'd say it's worth switching to |
Is there anything else I can do to help facilitate getting this through? 😄 |
LGTM :) |
Merged, thank you @Codex- :) |
Available in https://www.npmjs.com/package/@craco/craco/v/6.4.3 |
Great, thanks! |
Nah, just the defaults. Since >=7 introduces v2 of package lock I had it set to that but have lowered it so it shouldn't throw a warning for you now. I've cut a release now that should fix your issue, if you find you're still hitting 1.0.1 then remove craco and re-add it to resolve the new version. |
Apologies, what release version fixes this issue/how do I get that version? |
@seanblonien if you install the latest version of craco, 6.4.3, you will get the latest version of You can see it in your lock: "node_modules/cosmiconfig-typescript-loader": {
"version": "1.0.2",
"resolved": "https://registry.npmjs.org/cosmiconfig-typescript-loader/-/cosmiconfig-typescript-loader-1.0.2.tgz",
"integrity": "sha512-27ZehvijYqAKVzta5xtZBS3PAliC8CmnWkGXN0vgxAZz7yqxpMjf3aG7flxF5rEiu8FAD7nZZXtOI+xUGn+bVg==", If you find installing the latest craco does not pull this version, you will have it in your lock already, but this can be cleared by simply removing craco from your package, installing, then adding it back. |
Ahh it was a transitive dependency change/fix, that makes more sense. Thanks for fixing and explaining to me! It's working now ❤ |
With this change I am now getting these warnings during
|
If you're not using typescript / ts based config files you can disregard these, yarn also provides means to filter logs you're not interested in: https://yarnpkg.com/configuration/yarnrc#logFilters |
I've rewritten and published a replacement for the original
cosmiconfig-typescript-loader
package which addresses a number of issues reported on that original repository:#134
: "Doesn't work with Cosmiconfig sync API"#147
: "doesn't provide typescript, requested by ts-node"#155
: "Misleading TypeScriptCompileError when user's tsconfig.json "module" is set to "es2015""The main issue I was experiencing was the issue where cosmiconfig only works with transpiled modules that target commonjs, which is addressed with the replacement package. This issue is present with the current copied ts-loader code in craco.
See here for more details: https://github.com/Codex-/cosmiconfig-typescript-loader