-
-
Notifications
You must be signed in to change notification settings - Fork 476
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
After upgrade from 10.3.6 to 10.3.7 the eslint import plugin reports errors when importing glob #557
Comments
Same here |
@isaacs Do you think it's an issue with |
@ddolcimascolo i no think so, because locally i have glob package with version |
Cross reported in import-js/eslint-plugin-import#2883 just in case |
Looks like v10.3.6...v10.3.7 switched to building with tshy, but since v10.3.6 has "main", and v10.3.7 does not have "main", that's potentially a breaking change in a patch version. An argument can be made that since |
Thx for the explanation. @isaacs what's your opinion on this? |
Confirming #557 (comment) from @ljharb commit b7d7667 affects - "main": "./dist/cjs/src/index.js",
- "module": "./dist/mjs/index.js",
- "types": "./dist/mjs/index.d.ts", TypeScript also doesn't enable
Which shows up as this compiler error:
|
Given that TS by default breaks without |
temporary workaround: import { globSync } from 'glob/dist/esm'; Silences eslint errors, but next / typescript still complains. so I reverted the above change and update typescript config: "moduleResolution": "Bundler", Now it works |
dupe: #555 |
Enable resolvePackageJsonExports |
It does not solve the problem, because it force to change the option Could you fix the problem in |
I don’t think it makes sense to demand a typescript config change in a patch version. |
This isn't just a pedantic position, and it's not about me demanding anything. The You can enable As of TypeScript 5.2,
I am not the one demanding this. TypeScript is. All that I've done is stopped incorrectly advertising the wrong types to modules that are incorrectly importing this library without package.json exports enabled. If you were loading this module without exports support enabled, you already had a bug in your config. My bug was keeping yours hidden, and I fixed mine. |
@vitonsky You are already using configs that are compatible only with ts 5.1, and not 5.2. What's stopping you from changing |
@isaacs altho i'm not arguing this part is a breaking change, it would help out transitive users of |
@ljharb What has to happen to make I'd really like to only move forward with this, not backwards. Shipping incorrect types isn't good, and |
Quite a lot of effort, unfortunately, which is why it's still not yet done.
I totally agree, it's just that a single line in package.json seems like a small cost to pay to me for now ¯\_(ツ)_/¯ |
Is there a way to make it not a lot of effort? I just sent a PR draft, but I see now that you already had one that's been in progress for quite a while. Exports support was added in node 12, 7 years ago, and the last version of node to lack it was EOL'ed over 2 years ago. I know the goal is to support node versions back to the dark ages, but at this point, failing to support
The problem is that the "single line" is incompatible with |
If I understand the situation correctly: (I apologize if some of this has already been covered - I'm trying to verify my own understanding and see if everyone's on the same page. Please let me know if I've made any mistakes - corrections welcome.) First, 10.3.7 causes many common configurations to stop working. So, even if it resolves an issue with the types, it does so by breaking things for many other users. This seems like a semver-major breaking change to me. Second, in practice, I don't think anyone has complained about type issues in 10.3.6 and prior (even if they were technically incorrect). Third, the recommended solution of changing how tsc's configuration (i.e., setting Fourth, the resolve package is also affected, and updating that would also be quite a lot of work. Fifth: I don't think the types in 10.3.6 were actually broken. According to Are the types Wrong?:
If I understand correctly, the specific types issue that @isaacs is trying to address is described by "Are the types wrong?" as FalseCJS and FalseESM. However, I don't believe that the problems they describe apply to glob 10.3.6 - they say that the "golden rule of declaration files" is that you can't try to use one
As additional evidence, I tested every combination of tsconfig (Because package.json's |
The maintainers of `glob`, starting from version 10.3.7, removed the `types` entry from their package.json file. This change [broke](isaacs/node-glob#557) many of its consumers, including `jsii-compiler`. Following their suggestion, I changed the `module` and `moduleResolution` to node16. It was also necessary to rename `downlevel-dts.ts` to `downlevel-dts.d.ts`. --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0 --------- Signed-off-by: github-actions <github-actions@github.com> Co-authored-by: github-actions <github-actions@github.com>
Node 10 is not supported. It hasn't been supported for quite some time, since v8 3d6c4cd. The fact that it can't be loaded using the node10 module resolution is actually correct and intended.
If you are using node10 style module resolution with a hybrid module, it is impossible to guarantee that you are getting correct types. AreTheTypesWrong doesn't cover these scenarios. It is a valuable and useful tool, and I really like it, but the cases it covers are all situations in which the tsconfig matches the program loading the module. If I set If I set I understand that since glob does not use default exports or other features that will today trigger acute problems when loaded with the incorrect types in your programs, it might seem like a needlessly pedantic point. But:
The only way forward is to make your tsconfig match what your program is doing, or avoid using modules that use |
This isn't the fault of
For example, lots of tooling on GOV.UK Frontend can't use to
Sadly, these packages come from a very popular era of CommonJS with ES module types Until these packages update, TypeScript 5.2
But the release of |
I apologize for not breaking this sooner. That would have made this transition easier. Glob hasn't supported node v10 for a very long time now. It was a mistake to ever have If you are in fact using node 10, then you need to not be using this version of glob. If you are building for modern node, then you need to be telling typescript about it. Until you can get your program up to date with modern platforms that match and are in sync, the only safe way alternative is to be extremely cautious, pin all deps, only pull in updates after a careful review, etc. It sucks and is inconvenient, but it is what it is. |
Makes sense, yet is it too late to move the breaking change to v11? |
I mean, the presence of top level The question of whether it's even a breaking change is very debatable. I'd argue that if you are using a tool A that depends on tool B behaving incorrectly, it's not a breaking change to correct the behavior of tool B, even though doing so does require changes in tool A. The "breaking change" here is dropping support for node v10, but that already happened, 2 majors ago. If you have your toolchain configured to pretend to be node 10 (which is what it amounts to if you tsconfig |
Thanks for the reply, @isaacs. And thanks for your work on glob.
I understand that Node 10 isn't supported, but that isn't relevant to Much of your perspective seems to be "it's called
If not working with TypeScript and CommonJS resolution is correct and intended, then that's... surprising, considering that glob 10.x did everything I would expect a CJS-compatible module to do (no
Yes, it does. That's what its
Maybe this is contributing to our differing views? My tsconfig does match the program loading the module: I have a project that, for lots of reasons, still uses CommonJS, so I use
This is true, but it seems like a rare scenario? As I understand it, typical TypeScript usage is to use straight CommonJS ( The only way I've found to create the scenario you've described (main and types as CJS, load as ESM) is to force it - deliberately mix and match tsconfig.json options (e.g., That is a bug, but it's easy to work around (switch from default import to namespace import), and, like I said, I'm guessing it's a rare configuration to begin with. (Considering the huge number of tsconfig.json options that TypeScript exposes, it wouldn't surprise me if any nontrivial package can find multiple configurations that don't work; I'm not sure that automatically means a bug in those packages?) And glob 10.3.7 seems like a step backwards - from what I can tell, it went from "rare (?) combinations of options and use cases with CJS resolution don't work, with workarounds available" to "CJS resolution doesn't work."
Yes, that matches my understanding - main and types should use CJS, so that they can serve as the fallback for non-ESM-aware code and modes. Main and types as ESM would be incorrect.
This is your choice, of course, and I'll respect whatever you decide. But, if I understand correctly, this is effectively dropping CJS support (making glob ESM-only) for a good chunk of TypeScript users. I humbly suggest that that's a breaking change and that a good solution would be to restore 10.3.6's type definitions for 10.x then release the new approach as 11 (just like Sindre Sorhus did when he switched to ESM-only modules). Even if glob v8, v9, v10 were not intended to support CJS resolution for TS users, Hyrum's law seems to apply. And, again, thanks for your work. And I apologize for my long-winded post, and I apologize if I'm coming across as too argumentative. I'll accept whatever you decide, no need to reply unless you want to. |
* Fixes isaacs/node-glob#557, at least for me Signed-off-by: Marvin A. Ruder <signed@mruder.dev>
No apology needed, it's my own lack of clarity, but no, that is not my position. It's not the name that makes it obsolete, I don't care about that. It's the lack of support for "CJS module resolution", as you're calling it, is "use main, do not resolve exports". However, in practice, if you are not running your program in node 10, the actual behavior is "use exports, do not resolve main".
Lol, you're tellin me. 😅
You seem to be under the impression that glob no longer supports CommonJS. But it absolutely still does. Try The only reason it was not using However, when loading with
That is not correct. So CJS still works, you just have to tell TS that your program is using a version of node that resolves package.json exports.
TypeScript 5.2 makes this footgun harder to access, so this might not be allowed there, but in TS 5.1 and earlier, you could do this when package.json had top-level main/types fields: // tsconfig: { "module": "commonjs", "moduleResolution": "node" }
// very common setup!
import('glob').then(glob => {
glob
// ^ ESM glob at runtime, but TS sees CJS types, wrong
}) In this contrived example, it's not so bad. But it gets difficult to unwind when there's a chain of dependencies, the types are coming to you second-hand reexported, etc. |
I completely agree with @joshkel and would also suggest to "simply" delay this change to the next major release. |
Author of `glob` wants to break the world: isaacs/node-glob#557
For CommonJS packages with TypeScript default {
"compilerOptions": {
"paths": {
"glob": ["./node_modules/glob/dist/commonjs"]
}
}
} The compiler will discover |
@isaacs, thanks for your patient explanation. I had failed to realize that setting The challenge I'm running into (and much of the reason I was confused) is because it seems that several popular packages I'm using have broken package.json And I realize that none of this has to be any of your concern, but forcing downstream projects to deal with it all at once, as a result of one package's semver-patch upgrade, makes working through other packages' type issues trickier.
Understood. It's sure nice to not break existing code, though. 🙂 (Of course, there are always workarounds, such as renaming tap tests to .mts or putting a
I'm not sure that this is correct. If I compile the sample program you gave in glob 10.3.6, then |
Related discussion: isaacs/node-glob#557
Looking into this further, it seems that a commonjs main and You'll quite easily get the wrong types if you manage to have TS configured with But the goal of hybrid modules is maximal support for the greatest number of scenarios, so may as well exploit the loophole. |
Gah! It's definitely not "fine", that's terrible. Don't use those packages! The type information that you're getting from their top level |
Thanks @isaacs, all working now so that's great. Gives us time for our dependencies to fix their types |
@isaacs My problem has been solved. Thank you! |
report this error:
1:24 error Unable to resolve path to module 'glob' import/no-unresolved
The text was updated successfully, but these errors were encountered: