Fix - populate separate cache for require(esm) is a breaking change

[alexander-akait]

Version

24.8.0

Platform

any

Subsystem

No response

What steps will reproduce the bug?

In webpack we cache build dependencies (and nested require in any deps) and use this logic before (sinplified):

const module = require.cache[path];

if (module) { 
  // it is a common js module
  // we go through children and then also for their children and etc
} else if (/\.m?js$/.test(path)) {
  // it is ES modules or something else
  // here we use `es-module-lexer` to get all `import`s
}

Now this logic is broken, not only webpack uses this logic, it can break more tools

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

This change should be a part of the next major release, not minor

What do you see instead?

Logic is broken

Additional information

Ref - #59679

[alexander-akait]

Also with new logic we have no way to know if this is a ES module or CommonJS

[marco-ippolito]

@joyeecheung

[alexander-akait]

At a minimum, it would be nice to see an API or property indicating that an ES module was loaded with require, so it will be easy to handle

[joyeecheung]

It seems what would fix this is some way to properly tell whether the module is ESM or not other than using whether it's in require.cache or not? Because by definition require(esm) should put it there, and there's no documentation asserting that what's in there can only ever be commonjs (for example, it can already be addons). IMO it's more of an incorrect interpretation of what would be in the cache in the first place than a breaking change.

We already cache the detected format on the module, if necessary we can expose it. So that the branch would just be:

const module = require.cache[path];

if (module) { 
  if (module.format === 'module') {
  }
  if (module.format === 'commonjs') {
  }
} else if (/\.m?js$/.test(path)) {
  // it is something else
}

[alexander-akait]

@alexander-akait

It seems what would fix this is some way to properly tell whether the module is ESM or not other than using whether it's in require.cache or not?

Yes, it will allow to use resolve the problem

Because by definition require(esm) should put it there, and there's no documentation asserting that what's in there can only ever be commonjs (for example, it can already be addons).

Yes, I don't say it is a bug, I said that is a breaking change and should be part of the next major release, for esm we use custom lexer to parse dynamic imports and other dependency stuff, so this change breaks ES modules caches for all webpack users, I saw this logic in other tools (so not only in webpack broken), and the number of places can be much greater, and the problem can also be complex enough to understand why something doesn't work/doesn't cache/doesn't parsed

IMO it's more of an incorrect interpretation of what would be in the cache in the first place than a breaking change.

I wouldn't say so when all previous versions contained only the format, now there are different, which means the API has changed

We already cache the detected format on the module, if necessary we can expose it. So that the branch would just be:

Yes, it will fix the problem

[joyeecheung]

now there are different, which means the API has changed

In Node.js, things that are more in the "adding things that previously didn't exist" category are usually considered semver-minor (e.g., adding module.format would be in the similar category). It can be argued that every change is a breaking change and bug-for-bug compatibility hacks can be applied to make those who depend on undocumented behaviors effectively break less, though.

[joyeecheung]

This should reduce the impact by delaying the cache population until the ESM is directly required (which is necessary for correctness) #59874 - I'll open a different PR to add module.format to help dealing with ESM that actually gets required.