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

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

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

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.

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.

#559

Seems just setting main and types doesn’t actually solve the issue.

I think the only way forward here is to tell TSC to load modules properly.

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.

Confirming https://github.com/isaacs/node-glob/issues/557#issuecomment-1733405748 from @ljharb commit https://github.com/isaacs/node-glob/commit/b7d7667df0698b0e2c5589319b2b9b801eae35c0 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

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

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

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

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

Looks like https://github.com/isaacs/node-glob/compare/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.

Cross reported in https://github.com/import-js/eslint-plugin-import/issues/2883 just in case