After upgrade from 10.3.6 to 10.3.7 the eslint import plugin reports errors when importing glob

[doberkofler]

import {globSync} from 'glob';`

report this error:

   1:24  error    Unable to resolve path to module 'glob'                                          import/no-unresolved

[ddolcimascolo]

Same here

[vitonsky]

The same problem

[ddolcimascolo]

@isaacs Do you think it's an issue with eslint-plugin-import ?

[vitonsky]

@ddolcimascolo i no think so, because locally i have glob package with version 10.3.3 and all works fine

[ddolcimascolo]

Cross reported in import-js/eslint-plugin-import#2883 just in case

[ljharb]

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 engines.node is >=16 || 14 >=14.17, and all of those versions understand "exports" and don't need "main", that it's not breaking, but eslint-plugin-import doesn't yet understand "exports" (since resolve doesn't yet understand it), so that's what's causing the issue here.

[ddolcimascolo]

Thx for the explanation. @isaacs what's your opinion on this?

[colinrotherham]

Confirming #557 (comment) from @ljharb commit b7d7667 affects eslint-plugin-import by removing:

-  "main": "./dist/cjs/src/index.js",
-  "module": "./dist/mjs/index.js",
-  "types": "./dist/mjs/index.d.ts",

TypeScript also doesn't enable resolvePackageJsonExports by default:

true when moduleResolution is node16, nodenext, or bundler; otherwise false

Which shows up as this compiler error:

error TS2307: Cannot find module 'glob' or its corresponding type declarations

[ljharb]

Given that TS by default breaks without main, it does seem pretty widely breaking.

[AbdBarho]

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

[isaacs]

dupe: #555

[isaacs]

Enable resolvePackageJsonExports

[vitonsky]

Enable resolvePackageJsonExports

It does not solve the problem, because it force to change the option module. It is not acceptable for my project with electron https://github.com/vitonsky/deepink

Could you fix the problem in node-glob package, to not break the world and to not force other developers to do useless things because you have a principled position.

[ljharb]

I don’t think it makes sense to demand a typescript config change in a patch version.

[isaacs]

This isn't just a pedantic position, and it's not about me demanding anything.

The "types" field being listed in the top-level package.json field was not correct, because in TypeScript, the type of something exported as commonjs is different than something exported as esm. There's no way to make it actually work correctly for both with a single types field, if the package exports multiple main exports.

You can enable packageJsonExports support in TypeScript in a number of ways, but if you aren't enabling it somehow, then you're getting the wrong types for this module. And if you're not actually using a node version that supports package.json exports, then there's no way that you could actually be using this module anyway.

As of TypeScript 5.2, module and moduleResolution must be compatible. To get the correct types to load from this module you can:

• set module to node16 or nodenext and moduleResolution to the same value.
• set moduleResolution to bundler (I'm not familiar with all the particulars of this, because I don't use bundlers, but people are using it successfully with webpack and the like, I believe.)
• explicitly set resolvePackageJsonExports: true

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.

[isaacs]

@vitonsky You are already using configs that are compatible only with ts 5.1, and not 5.2. What's stopping you from changing moduleResolution from node to node16? That would resolve this issue, with the added benefit of your build getting the correct type information.

[ljharb]

@isaacs altho i'm not arguing this part is a breaking change, it would help out transitive users of resolve if you re-added main to package.json.

[isaacs]

@ljharb What has to happen to make resolve able to interpret exports in package.json? Can it use https://www.npmjs.com/package/resolve-import?

I'd really like to only move forward with this, not backwards. Shipping incorrect types isn't good, and main is an affordance only needed for very old node versions that aren't supported by any of my code anyway.

[ljharb]

Quite a lot of effort, unfortunately, which is why it's still not yet done.

an affordance only needed for very old node versions that aren't supported by any of my code anyway.

I totally agree, it's just that a single line in package.json seems like a small cost to pay to me for now ¯\_(ツ)_/¯

[isaacs]

@ljharb

Quite a lot of effort, unfortunately, which is why it's still not yet done.

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 exports feels like it's not living up to being a correct implementation of require.resolve. It strikes me as a weird choice to support node <=10 behavior instead of the current active LTS versions (and even the LTS version that's been dead a year).

I totally agree, it's just that a single line in package.json seems like a small cost to pay to me for now ¯_(ツ)_/¯

The problem is that the "single line" is incompatible with "type": "module" if it's commonjs, so it's still either a footgun of subtly incorrect types, or reverting all my tests, bins, etc, to commonjs, or still a breaking change anyway, since anyone resolving to that main entry and then trying to require() it will throw an error. The easiest and most consistent approach overall is to ditch top level main and types, and use the modern export mechanism.

[joshkel]

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 "moduleResolution": "node16" or setting "resolvePackageJsonExports": true) is a major change for projects using glob - requiring, to some extent, converting each project from CommonJS to ESM. That's quite a lot of work.

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?:

• 10.3.6 was good.
• 10.3.7 and 10.3.9 fail to resolve types using TypeScript's "node" / "node10" resolution mechanism (the issue we're discussing here).
• 10.3.8 doesn't raise errors on "Are the types wrong?", but it introduced another error (see Compile error with 10.3.8 #559, which I'm not able to reproduce locally). It also changes its own package.json type from CommonJS to module, which I thought was a breaking change.

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 .d.ts file to cover two different (e.g., CommonJS and ESM) .js files, because it will cause problems for "moduleResolution": "node16". However, glob 10.3.6 didn't do that:

• If you're using "moduleResolution": "node10", then tsc evaluates 10.3.6's "main" and "types" fields (and, as I understand it, ignores 10.3.6's "module" field - that's a feature that bundlers added for ESM support before Node.js standardized on "exports", and it's ignored by Node). In this mode, tsc doesn't support ESM, so there's no possibility of a mismatch between CommonJS and ESM. (Note that 10.3.6's actual .d.ts files are identical across the CommonJS and ESM builds - therefore, using an ESM .d.ts file with a CJS .js file is okay, as long as tsc doesn't become confused about which type it thinks each file is.)
• If you're using "moduleResolution": "node16" (which enables tsc's ESM support, so tsc now has to care about ESM versus CJS), then you've also enabled using package.json exports. 10.3.6's package.json correctly uses separate types within its package.json exports for "import" versus "require", so it was fine.

As additional evidence, I tested every combination of tsconfig "module" / "moduleResolution", default versus namespace import of glob, and top-level package.json "type": "module" versus CommonJS for glob 10.3.6 and 10.3.7 that I could think of, and I could not find any circumstances where 10.3.6 had type errors or where 10.3.6 worked worse than 10.3.7.

(Because package.json's "module" field is a bundler feature, I should add that I did not attempt to test bundlers. Since glob is a Node.js module and doesn't make much sense in a browser, Rollup is probably the only bundler that applies here? I'm less familiar with it, but @rollup/plugin-typescript uses TypeScript, and Rollup ought to support package.json exports, so I'd expect it to act the same as tsc.)

[isaacs]

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.

In this mode, tsc doesn't support ESM, so there's no possibility of a mismatch between CommonJS and ESM.

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 main and types to cjs, and you load the module from ESM or via async import, then ts in moduleResolution:'node10' mode will compile the code using the CJS types, but in fact, the ESM will be loaded at run time.

If I set main and types to esm, and you are compiling in node10 resolution mode, then you'll get an errors like #559 (among other things). It apparently doesn't respect "type": "module" in package.json in that mode, so there's no way to give it a main that isn't commonjs, in node10 module/moduleResolution mode.

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:

1. this is a bug waiting to happen, I have run into several issues as a result of this sort of discrepancy in the code I've been writing and consuming,
2. at best, it's going to make tsc needlessly wrap the import in its esModuleInterop code, which is suboptimal, and
3. I'm trying to make my corpus of open source manageable at scale, and that is only possible by relying on consistent patterns that can be made universal across the modules I maintain.

The only way forward is to make your tsconfig match what your program is doing, or avoid using modules that use exports in package.json to ship hybrid support for both CJS and ESM.

[colinrotherham]

This isn't the fault of glob but lots of popular incorrectly-typed packages now don't work alongside it:

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'

For example, lots of tooling on GOV.UK Frontend can't use to Node NodeNext as shown here:

• Problems with @rollup/plugin-terser
• Problems with @rollup/plugin-replace
• Problems with @percy/puppeteer
• Problems with filesize

Sadly, these packages come from a very popular era of CommonJS with ES module types

Until these packages update, TypeScript 5.2 "moduleResolution": "Node10" supports them:

'node10' (previously called 'node') for Node.js versions older than v10, which only support CommonJS require. You probably won’t need to use node10 in modern code.

But the release of glob@10.3.7 does lose "moduleResolution": "Node10" compatibility:
https://github.com/alphagov/govuk-frontend/actions/runs/6310135700/job/17131564591

[isaacs]

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 main and types at the top level of the package.json while shipping hybrid with exports. I did not realize the implications of that at the time, but thankfully (albeit somewhat annoyingly), typescript 5.2 has forced the issue.

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.

[colinrotherham]

Makes sense, yet is it too late to move the breaking change to v11?

[isaacs]

I mean, the presence of top level main and types is a bug in v10, v9, and v8. The real question is, should it be backported to the last 2 majors? (The answer is no, I ain't got time for backporting to old majors lol.)

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 "moduleResolution":"node10" and rely on tools that only support node 10 loading semantics), and that's breaking, well, node 10 isn't supported, hasn't been for a long time, so if you're pretending to be node 10, things might not work.

[joshkel]

Thanks for the reply, @isaacs. And thanks for your work on glob.

Node 10 is not supported. It hasn't been supported for quite some time, since v8 3d6c4cd.

I understand that Node 10 isn't supported, but that isn't relevant to "moduleResolution": "node10". "node10" is what TypeScript used to call "moduleResolutionNode": "node"; it's the original, pre-ESM (so CommonJS) Node.js module handling. In other words, "node10" doesn't mean "pretend to be Node 10"; it means "stick with CJS module resolution, as supported by Node 10 (even though lots of other more modern packages also still use CJS, for a variety of reasons)." There are still large portions of the ecosystem on CJS (see, e.g., here); it's not legacy in the same way that Node.js 10.x itself is.

Much of your perspective seems to be "it's called node10, so it's obsolete." (If I've misunderstood your position, I apologize.) But Node 20 itself still fully supports Node 10-compatible CommonJS resolution. Using an older interface / standard that's compatible with Node 10 isn't the same thing as pretending to be Node 10. Node 20 still doesn't provide full equivalents for ESM that it did for CJS (as seen in some of Jest's issues), and so "node10" doesn't mean "obsolete since Node 12 was released."

The fact that it can't be loaded using the node10 module resolution is actually correct and intended.

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 "type": "module", a CJS "main", a "types" that caused no reported issues for CJS users, and a cjs directory). But that's your prerogative.

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.

Yes, it does. That's what its node10 line is checking.

The cases it [AreTheTypesWrong] covers are all situations in which the tsconfig matches the program loading the module.

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 "module": "commonjs", "moduleResolution": "node10". So tsc expects to load CJS code at compile time, and it transpiles to require so that Node.js loads CJS code at runtime. Since glob 10.3.6 shipped both CJS and ESM code and shipped types compatible with tsc-expecting-CJS, everything worked.

If I set main and types to cjs, and you load the module from ESM or via async import, then ts in moduleResolution:'node10' mode will compile the code using the CJS types, but in fact, the ESM will be loaded at run time.

This is true, but it seems like a rare scenario? As I understand it, typical TypeScript usage is to use straight CommonJS ("module": "commonjs", "moduleResolution": "node10") or straight ESM ("module": "node16', "moduleResolution": "node16"). As I described in my earlier comment, main and types as CJS works just fine in this case.

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., "module": "esnext", "moduleResolution": "node10"), and use a default import (import glob from 'glob') (because ESM and CJS are otherwise too similar for type differences to cause an issue), and enable esModuleInterop (because tsc otherwise complains that it can't find a default export in glob 10.3.6).

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."

If I set main and types to esm, and you are compiling in node10 resolution mode, then you'll get an errors like #559 (among other things). It apparently doesn't respect "type": "module" in package.json in that mode, so there's no way to give it a main that isn't commonjs, in node10 module/moduleResolution mode.

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.

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...

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.

[isaacs]

Much of your perspective seems to be "it's called node10, so it's obsolete." (If I've misunderstood your position, I apologize.)

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 exports that is a problem.

"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".

(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;

Lol, you're tellin me. 😅

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 "type": "module", a CJS "main", a "types" that caused no reported issues for CJS users, and a cjs directory). But that's your prerogative.

You seem to be under the impression that glob no longer supports CommonJS. But it absolutely still does. Try require('glob') in a repl, you'll see it works just fine in any node version going back to v12.

The only reason it was not using "type": "module" is that my tests were written in CJS. And that, only because it was using a version of tap that didn't support ESM. Tap 18 does support ESM, and it's a more ergonomic way to write tests, so that's what I'm moving towards. The package.json files in dist/{commonjs,esm} set the module type for the distributed modules, assuming that they are resolved via the exports object, which they are at run-time, and so the "type" at the top level of the package ought to be irrelevant to your build, because it is irrelevant to your actual build target.

However, when loading with "moduleResolution": "node10", it does not parse exports, it only looks at main, because you are telling TS that you are building for an environment that supports looking up package names in node_modules which have a CommonJS main or index.js, but not resolving exports. This results in 4 possible confusing/suboptimal situations:

• main pointing to ESM + "type": "module" AFAICT, this makes TS try to load the types as CJS, and fail when on the types in dist/esm/...
• main pointing to CJS + "type":"commonjs" Unnecessarily restrictive, running tests in CJS mode, loading code from CJS. This is annoying because while ESM code can load CJS no problem, loading ESM from CJS is a pita.
• main pointing to ESM + "type": "commonjs" Incorrect, not sure what the outcome would be. But it has the same ergonomic issues with testing and building.
• main pointing to CJS + "type": "module" Incorrect, unsure what the outcome would be. But maybe this one could work? I suspect that it may cause problems with any node version that doesn't support exports but does support "type":"module" (if there are any), but since it's clear idgaf about node pre-14 (usually pre-16), maybe that's worth trying?

But, if I understand correctly, this is effectively dropping CJS support (making glob ESM-only) for a good chunk of TypeScript users.
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."

That is not correct. moduleResolution: node16 has been around about as long as node 16, and if you are using that setting, then it will know to load the CJS or ESM code and types appropriately, because it will properly resolve using the exports object, as every version of node shipped in the last 7 years has done, and as your program almost certainly does at run-time.

So CJS still works, you just have to tell TS that your program is using a version of node that resolves package.json exports.

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

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.

[doberkofler]

I completely agree with @joshkel and would also suggest to "simply" delay this change to the next major release.

[colinrotherham]

For CommonJS packages with TypeScript default "moduleResolution": "Node10" here's a workaround:

{
  "compilerOptions": {
    "paths": {
      "glob": ["./node_modules/glob/dist/commonjs"]
    }
  }
}

The compiler will discover index.js + index.d.ts automatically

[joshkel]

@isaacs, thanks for your patient explanation. I had failed to realize that setting "moduleResolution": "Node16" did not automatically enable ESM, so that's a major mistake in my previous comment.

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 "exports" types. As long as their top-level "main" and "types" are correct and package.json exports can resolve to CJS, this is fine. The result of the glob changes still kinda feels like dropping support for TS with CommonJS (because, as a result of all these bad exports types, sticking with CommonJS and "moduleResolution": "node10" appears to be the most reliable setup), and it seems that the ecosystem is a good several years behind the "package.json exports is mature and standardized, so just use it" ideal that you're describing, but my technical understanding of the details was wrong.

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.

main pointing to CJS + "type":"commonjs" Unnecessarily restrictive, running tests in CJS mode, loading code from CJS. This is annoying because while ESM code can load CJS no problem, loading ESM from CJS is a pita.

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 { "type": "module" } directory in the test folder - but, again, that's your prerogative, no justification needed.)

TS 5.1 and earlier, you could do this when package.json had top-level main/types fields...

I'm not sure that this is correct. If I compile the sample program you gave in glob 10.3.6, then import('glob') is converted to Promise.resolve().then(() => require('glob')). (Even if it did use ESM imports, glob's ESM and CJS types are the same, so it doesn't appear to be an issue for glob. As you mentioned, other scenarios, such as default exports, may be trickier, and I think this is turning into a tangent from the original issue.)

[isaacs]

Looking into this further, it seems that a commonjs main and type:module is actually not a hazard for any node version that can parse the actual code here, so that's fine.

You'll quite easily get the wrong types if you manage to have TS configured with "module" set to "node16", "nodenext", "es2022", or "esnext" along with "moduleResolution": "node", but thankfully, TS 5.2 does not allow that any more. It's somewhat concerning to me that there are people who've reported just such a setup being broken by this change, which is part of why I've been so stubborn about it; breaking their build is actually the ethical thing to do in that case, because it's currently running broken.

But the goal of hybrid modules is maximal support for the greatest number of scenarios, so may as well exploit the loophole.

[isaacs]

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 "exports" types. As long as their top-level "main" and "types" are correct and package.json exports can resolve to CJS, this is fine.

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 types and main field isn't corresponding to what your program actually loads. That means that your libraries that use those packages will likely export incorrect types as well, if you expose any of their types.

[colinrotherham]

Thanks @isaacs, all working now so that's great. Gives us time for our dependencies to fix their types

[doberkofler]

@isaacs My problem has been solved. Thank you!