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

Import failing after upgrade to v5 #306

Closed
CarlosNZ opened this issue Oct 6, 2023 · 26 comments
Closed

Import failing after upgrade to v5 #306

CarlosNZ opened this issue Oct 6, 2023 · 26 comments

Comments

@CarlosNZ
Copy link

CarlosNZ commented Oct 6, 2023

Package has been working fine with v4.1.2

However, after upgrading to v5, I get the following error:

Cannot find module 'change-case' or its corresponding type declarations.

My import command is just import { camelCase } from 'change-case', which worked fine before. 🤷‍♂️

@CarlosNZ
Copy link
Author

CarlosNZ commented Oct 6, 2023

Indeed, it would appear I'm not the only one. This is what I get when I try to look up the package in Bundlephobia:

Screenshot 2023-10-06 at 5 26 58 PM

https://bundlephobia.com/package/change-case

This is fine: https://bundlephobia.com/package/[email protected]

@blakeembrey
Copy link
Owner

blakeembrey commented Oct 7, 2023

It’s a duplicate of #304. Unfortunately Bundlephobia doesn’t support ESM so you can’t rely on that result. Please see the note in the READMEs of the packages on how to enable ESM, I don’t support CommonJS anymore.

@olistrik
Copy link

olistrik commented Oct 9, 2023

I also have this issue with an angular 16 project. Changing the moduleResolution to node16, esmNext, or bundler, breaks all of our imports.

@blakeembrey
Copy link
Owner

If you have a reproduction I can look at I can help you debug, but that sounds more like a bug in angular. Typescript support for ESM is just following https://www.typescriptlang.org/docs/handbook/esm-node.html

@emanuel-lundman
Copy link

Might also be a problem with moduleResolution being set to node in tsconfig in the projects trying to use change-case. If you’ve got a recent version of typescript one could probably switch to bundler instead and keep approximately the same behaviour, but importing change-case would start working, it worked when I tried it out.
This is because change-case package.json doesn’t include a module field pointing to the dist folder, and ts projects still using node as moduleResolution will fail to find the types. They don’t seem to know anything about the exports field.

change-case could add it, it won’t do any harm to the project I guess, and it would work for those old typescript projects as well, probably a lot of projects not changing the moduleResolution that often. Or perhaps hint at the solution in the docs for anyone trying to figure it out?

I noticed it in a project that uses a lot of other esm-packages, but types for change-case didn’t work.

@olistrik
Copy link

I've found that I needed to do three things to get it just about working:

  1. upgrade typescript to >=5. (but < 5.2 because angular)
  2. use 'moduleResolution': 'bundler'. All others fail.
  3. set 'type': 'module' in the package.json

However all of our tests now fail because Karma doesn't support ESM imports in their conf.js and ESM doesn't support requires. We're also not too comfortable changing the moduleResolution unless we can guarantee the same behavior and only using change-case for case insensitive splitting in a few places, so messing with the moduleResolution` and all the changes that brings seems a little excessive when we could just split the strings another way.

@IGx89
Copy link

IGx89 commented Oct 12, 2023

We use Vuex 4 which doesn't support 'moduleResolution': 'bundler' either, so back to pascal-case it is for us :(.

@JakeAi
Copy link

JakeAi commented Oct 13, 2023

@blakeembrey this esm change breaks my entire nx monorepo...

@blakeembrey
Copy link
Owner

blakeembrey commented Oct 13, 2023

@JakeAi No it doesn’t. You aren’t being forced to upgrade. That’s the point of a breaking change version bump, it introduces incompatible changes with the previous version. I no longer intend to maintain CommonJS packages.

@blakeembrey
Copy link
Owner

change-case could add it, it won’t do any harm to the project I guess, and it would work for those old typescript projects as well, probably a lot of projects not changing the moduleResolution that often. Or perhaps hint at the solution in the docs for anyone trying to figure it out?

No, adding it won’t fix typescript. I’d need to create a CommonJS compatible version of the package, therefore shipping two versions. I did that for the last few years with the previous major version, going forward it’ll be ESM only.

@blakeembrey
Copy link
Owner

Also, there is documentation added to each of the packages READMEs, can you let me know if that is insufficient?

@JakeAi
Copy link

JakeAi commented Oct 13, 2023

I create a blank nx repo, with an angular app. Import your library. And change-case package is not found. I add "module": "node16", "moduleResolution": "node16" and angular libraries break and I get warned about the current file being CommonJS. I add "type": "module", and I get

./apps/mm-site/src/app/nx-welcome.component.ts:8:22-44 - Error: Module not found: Error: Package path . is not exported from package C:\Development\mm\node_modules\change-case (see exports field in C:\Development\mm\node_modules\change-case\package.json)

./apps/mm-site/src/app/nx-welcome.component.ts:9:40-67 - Error: Module not found: Error: Package path ./keys is not exported from package C:\Development\mm\node_modules\change-case (see exports field in C:\Development\mm\node_modules\change-case\package.json)

@blakeembrey
Copy link
Owner

I’m sorry, it’s ESM only now and apparently that doesn’t work properly with Angular. I’d suggest filing a ticket with them, I’m not the only person building packages with ESM nowadays.

change breaks my entire nx monorepo

I think I misinterpreted this to assume you meant an existing repo. Of course, you can still install the previous major release. You don’t need the latest version.

@blakeembrey
Copy link
Owner

@JakeAi If there’s anything you can share with me to reproduce I can file an issue on your behalf if you’d prefer. I’m not familiar with NX or Angular.

@blakeembrey
Copy link
Owner

It could be related to something like this? nrwl/nx#18801. Or maybe this? nrwl/nx#15464.

TL;DR is that it seems like NX is doing some sort of “magic” that just doesn’t work properly with ESM + TypeScript here. Unfortunately I’m not too familiar with this, I typically avoid using tools like this and use direct tsc and/or a simple bundler like ESBuild.

@blakeembrey
Copy link
Owner

Also nrwl/nx#11335, seems like it’s been over a year and people can’t run ESM using NX 🤷 It doesn’t sound promising.

@emanuel-lundman
Copy link

emanuel-lundman commented Oct 13, 2023

change-case could add it, it won’t do any harm to the project I guess, and it would work for those old typescript projects as well, probably a lot of projects not changing the moduleResolution that often. Or perhaps hint at the solution in the docs for anyone trying to figure it out?

No, adding it won’t fix typescript. I’d need to create a CommonJS compatible version of the package, therefore shipping two versions. I did that for the last few years with the previous major version, going forward it’ll be ESM only.

No. I was not referring to the ESM switch. I was referring to adding the module field in package.json which would fix this issue for all projects using a tsconfig with moduleResolution set to node which probably is a lot. When set to node ts don't understand the exports field in package.json according to my understanding. Which gives the problem described in this issue.
I had a project using multiple ESM packages and it's an ESM itself. But this issue was still present due to the missing module field in package.json pointing at the dist folder. If you don't want to add it for some reason maybe it could be documented as a troubleshooting tip to others having the missing ts types issue to check if it's possible to switch moduleResolution setting?

@blakeembrey
Copy link
Owner

I was referring to adding the module field in package.json which would fix this issue for all projects using a tsconfig with moduleResolution set to node which probably is a lot.

This doesn’t fix typescript, it’s not how typescript works. How did you get this to work?

@blakeembrey
Copy link
Owner

There’s nowhere in TypeScript’s module resolution strategies that it even uses the module field, as far as I’m aware. Some bundler may consider it, but what TypeScript actually uses in the old node module resolution strategy is the types/typings field. And that is expected to be the CommonJS version of the package. If I added that back, it wouldn’t work properly with imports because it’d allow CommonJS in TypeScript to work differently depending on the users synthetic import options. And then I’d just have a different set of issues being filed asking for CommonJS to work.

@gwsbhqt
Copy link

gwsbhqt commented Oct 14, 2023

My project still doesn't work, it's been a whole day trying to fix this. :(

@emanuel-lundman
Copy link

There’s nowhere in TypeScript’s module resolution strategies that it even uses the module field, as far as I’m aware. Some bundler may consider it, but what TypeScript actually uses in the old node module resolution strategy is the types/typings field. And that is expected to be the CommonJS version of the package. If I added that back, it wouldn’t work properly with imports because it’d allow CommonJS in TypeScript to work differently depending on the users synthetic import options. And then I’d just have a different set of issues being filed asking for CommonJS to work.

Don’t know, might have changed something else aswell.
I’m not an expert at this but the module resolution thing in ts is an issue when only exportsfield is available in the package.json, since many of the settings doesn’t seem to work.

Tried in a mini repro:

package.json

{
  "type": "module",  
  "devDependencies": {
    "ts-node": "^10.9.1",
    "typescript": "^5.2.2"
  },
  "dependencies": {
    "change-case": "^5.0.2"
  },
  "scripts": {
    "start": "ts-node-esm index.ts"
  }
}

tsconfig.json

{
  "compilerOptions": {
    "target": "es2016",
    "module": "ESNext",
    "moduleResolution": "node",
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "strict": true,
    "skipLibCheck": true
  }
}

index.ts

import * as changeCase from "change-case";

console.log(changeCase.camelCase("what is happening"));

This above e.g. will say types are missing.

Changing module and moduleResolution in tsconfig to:

    "module": "NodeNext",
    "moduleResolution": "NodeNext",

Will make it work.

And manually editing change-cases package.json and adding:

"types":  "dist/index.js",

Will also make it work.

There might (and are) more ways to make it work. I used another combo in our bigger project.
But since there seem to be a lot of tsconfig settings preventing typescript from finding the types when only exports is available in the package.json it might be an easy solution to add e.g. types or typesVersions field(s) or something else helping more moduleResolution settings finding the types. :) without signaling anything about commonjs.

Some other esm packages seem to have both ”types” and ”exports” e.g.:
https://www.npmjs.com/package/@sindresorhus/is
https://github.com/sindresorhus/is/blob/main/package.json

@blakeembrey
Copy link
Owner

@emanuel-lundman Thanks for the references. FWIW I wanted to check why they added types since it's technically incorrect for TypeScript and he's saying the same thing here: sindresorhus/is#180.

@blakeembrey
Copy link
Owner

blakeembrey commented Oct 18, 2023

Specifically:

I will revert it for now. But it will come back in the next major version.

The problem is your TypeScript config. Make sure you'are on TS 5 or later and use this config: https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c#how-can-i-make-my-typescript-project-output-esm

(The gist link is the same one I've added to the package READMEs to clarify this too).

@emanuel-lundman
Copy link

I don't quite follow if we're talking about the same thing. But that's alright. Thanks for looking in to it. 🙂

Just to be clear, I wasn't saying there's a need to revert the exports field in package.json to make it work. But adding the types field seemed during my test to make it much more compatible with more moduleResolutions than node16.

Switching to moduleResolution node16 can be a big hassle for an old large project. Adding extensions to all imports maybe in hundreds of files etc.
And seems like typescript still promotes to add it to the package.json. https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

TLDR;
Adding types field to package.json seems to me like it could improve compatibility and make typescript find the types in more situations and it's such a minor thing.
But of course, my testing hasn't been that exhaustive.

The package.json I referenced in the sindresorhus package e.g. was for the next major version (6.0.1) and he had reverted to using exports again but he still had a types field in his package.json.

   ...
	"type": "module",
	"exports": "./dist/index.js",
	"types": "./dist/index.d.ts",
	"engines": {
		"node": ">=16"
	},
   ...

But of course it's up to you to decide. Wish you all the best. 🙂

@blakeembrey
Copy link
Owner

blakeembrey commented Oct 19, 2023

But adding the types field seemed during my test to make it much more compatible with more moduleResolutions than node16.

Yes, I agree. It works for TypeScript, but it won't work for node projects using CommonJS. I vaguely understand wanting to allow people to just upgrade and it work, but to be fair it'd just break elsewhere. I.e. the error would turn into a node.js issue trying to require an ESM package. Arguably it's "better", but I'd personally prefer to contribute to upgrading the NPM ecosystem than comfort. And we have found and can fix the TypeScript compiler message to improve this experience now too.

was for the next major version (6.0.1) and he had reverted to using exports again but he still had a types field in his package.json.

It doesn't look like he reverted again, just forgot to revert before 6.x 🤷 Easy to forget I suppose, but I see what you mean now. I guess there's other reasons he might have left it alone, I've found other ecosystem interop issues too (i.e. NPM package pages also don't support ESM types exports).

I'll put my effort to improving the ecosystem rather than hacking it in this package, hopefully we can make it so this is less confusing on future TypeScript versions.

@blakeembrey
Copy link
Owner

@emanuel-lundman I thought more about your point overnight and added types back for all the packages. Hopefully it alleviates some of the TypeScript pain, but it doesn't add CommonJS so it may introduce a different set of confusion. I'll make a backlog of the issues flagged from this about the ESM ecosystem and maybe myself or someone else can help improve the tooling:

  1. Improve the TypeScript error to indicate it's an ESM package, instead of Cannot find module 'change-case' or its corresponding type declarations. Better error message for using outdated moduleResolution: node(10)  microsoft/TypeScript#55104
  2. Fix NPM unable to detect "types" for header when using ESM (TODO: Find repo and issue).
  3. Investigate an alternative to BundlePhobia that supports ESM Support for multiple entry points with "exports" pastelsky/bundlephobia#379

In another issue someone shared this and it was very helpful way to debug TypeScript/ESM support: https://arethetypeswrong.github.io

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

No branches or pull requests

7 participants