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

Fix: Use rewritten cosmiconfig-typescript-loader package for handling TS config files. #347

Merged
merged 1 commit into from
Dec 9, 2021

Conversation

Codex-
Copy link
Contributor

@Codex- Codex- commented Nov 30, 2021

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

- Fixes issue where incompatible module types are loaded implicitly.
@patricklafrance
Copy link
Contributor

Before merging this PR, I would like to have your opinion @alvis.

@Codex-
Copy link
Contributor Author

Codex- commented Dec 1, 2021

I've tested this on my own usage of craco, but it would be great to have some additional sanity tests done too 👍

@alvis
Copy link
Contributor

alvis commented Dec 3, 2021

Hi @Codex- Thanks for making cosmiconfig-typescript-loader. It's always good to someone to step up and improve the original unmaintained loader. 👍🏼

However, there are few points I'm not sure adopting cosmiconfig-typescript-loader here is good move.

@Codex-
Copy link
Contributor Author

Codex- commented Dec 4, 2021

It's worth noting that the current implementation introduces a dependency on ts-node without explicitly adding it as a package dependency, so it is depended upon implicitly and can fail in repositories that do not have this dep present.

Craco now also suffers from #155. If it's not CJS, then consequently it is not compatible with the current implementation of cosmiconfig.

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 esm is not compatible with cosmiconfig.

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.

@alvis
Copy link
Contributor

alvis commented Dec 4, 2021

It's worth noting that the current implementation introduces a dependency on ts-node without explicitly adding it as a package dependency, so it is depended upon implicitly and can fail in repositories that do not have this dep present.

Thanks for pointing out. It's true and it's a mistake should be fixed.

Craco now also suffers from #155. If it's not CJS, then consequently it is not compatible with the current implementation of cosmiconfig.

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 cosmiconfig-typescript-loader. :)

@Codex-
Copy link
Contributor Author

Codex- commented Dec 8, 2021

Is there anything else I can do to help facilitate getting this through? 😄

@alvis
Copy link
Contributor

alvis commented Dec 9, 2021

LGTM :)

@patricklafrance patricklafrance merged commit ab22104 into dilanx:master Dec 9, 2021
@patricklafrance
Copy link
Contributor

Merged, thank you @Codex- :)

@patricklafrance
Copy link
Contributor

Available in https://www.npmjs.com/package/@craco/craco/v/6.4.3

@Codex-
Copy link
Contributor Author

Codex- commented Dec 9, 2021

Great, thanks!

@seanblonien
Copy link

So we have to use npm 7 or greater now?
image
I would love too but one of my dependencies has a breaking peer dependency specificity that prevents me from upgrading NPM versions. Guess I'll be on Craco 6.4.2 until then

@Codex- Codex- deleted the cfg_ts_loader branch December 22, 2021 02:05
@Codex-
Copy link
Contributor Author

Codex- commented Dec 22, 2021

So we have to use npm 7 or greater now?

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.

@seanblonien
Copy link

Apologies, what release version fixes this issue/how do I get that version?

@Codex-
Copy link
Contributor Author

Codex- commented Dec 28, 2021

@seanblonien if you install the latest version of craco, 6.4.3, you will get the latest version of cosmiconfig-typescript-loader.

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.

@seanblonien
Copy link

Ahh it was a transitive dependency change/fix, that makes more sense. Thanks for fixing and explaining to me!

It's working now ❤

@nmoinvaz
Copy link

With this change I am now getting these warnings during yarn install:

warning "@craco/craco > cosmiconfig-typescript-loader@1.0.4" has unmet peer dependency "@types/node@*".
warning "@craco/craco > cosmiconfig-typescript-loader@1.0.4" has unmet peer dependency "typescript@>=3".
warning "@craco/craco > cosmiconfig-typescript-loader > ts-node@10.4.0" has unmet peer dependency "@types/node@*".
warning "@craco/craco > cosmiconfig-typescript-loader > ts-node@10.4.0" has unmet peer dependency "typescript@>=2.7".

@Codex-
Copy link
Contributor Author

Codex- commented Jan 24, 2022

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

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.

5 participants