Skip to content

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

Closed as not planned
Closed as not planned
Fix - populate separate cache for require(esm) is a breaking change#59868
Labels
moduleIssues and PRs related to the module subsystem.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

Description

@alexander-akait
alexander-akait
opened on Sep 12, 2025

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

Activity

  1. alexander-akait commented on Sep 12, 2025

    @alexander-akait
    alexander-akait
    on Sep 12, 2025
    Author

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

  2. marco-ippolito
    added
    moduleIssues and PRs related to the module subsystem.
    on Sep 12, 2025

  3. marco-ippolito commented on Sep 12, 2025

    @marco-ippolito
    marco-ippolito
    on Sep 12, 2025
    Member

    @joyeecheung

  4. alexander-akait commented on Sep 12, 2025

    @alexander-akait
    alexander-akait
    on Sep 12, 2025
    Author

    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

  5. joyeecheung commented on Sep 12, 2025

    @joyeecheung
    joyeecheung
    on Sep 12, 2025
    Member

    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
}

  6. alexander-akait commented on Sep 12, 2025

    @alexander-akait
    alexander-akait
    on Sep 12, 2025
    Author

    @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

  7. joyeecheung commented on Sep 12, 2025

    @joyeecheung
    joyeecheung
    on Sep 12, 2025
    Member

    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.

  8. joyeecheung commented on Sep 12, 2025

    @joyeecheung
    joyeecheung
    on Sep 12, 2025
    Member

    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.

  9. nodejs-github-bot
    added a commit that references this issue on Sep 17, 2025

    module: only put directly require-d ESM into require.cache

    c462602
  10. targos
    added 2 commits that reference this issue on Sep 18, 2025

    module: only put directly require-d ESM into require.cache

    5fac3ce

    module: only put directly require-d ESM into require.cache

    728472a

  11. ChALkeR commented on Sep 26, 2025

    @ChALkeR
    ChALkeR
    on Sep 26, 2025
    Member

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

    That's not (entirely) true:

    > require('./1.mjs')
[Module: null prototype] {  }
> require('./1.cjs')
{}
> require.cache[require.resolve('./1.mjs')].exports?.[Symbol.toStringTag] === 'Module'
true
> require.cache[require.resolve('./1.cjs')].exports?.[Symbol.toStringTag] === 'Module'
false

    Will misdetect on module.exports = require('./1.mjs') though (i.e. when the whole exports is a re-export of ESM)

    More hacky:

    > require('./1.cjs')
[Module: null prototype] {  }
> require('./1.mjs')
[Module: null prototype] {  }
> const kFormat = Object.getOwnPropertySymbols(require.cache[require.resolve('./1.cjs')]).find(s => s.description === 'kFormat')
undefined
> require.cache[require.resolve('./1.cjs')][kFormat]
'commonjs'
> require.cache[require.resolve('./1.mjs')][kFormat]
'module'

    Note: this is not a recommendation to use that, just noting that "no way" is not technically correct

  12. github-actions commented on Apr 25, 2026

    @github-actions
    github-actionsbot
    on Apr 25, 2026 – with GitHub Actions
    Contributor

    This issue has been marked as stale due to 210 days of inactivity.
    It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

  13. github-actions
    added
    staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.
    on Apr 25, 2026
  14. marco-ippolito
    added a commit that references this issue on Apr 27, 2026

    module: only put directly require-d ESM into require.cache

    12a2462

  15. github-actions commented on May 25, 2026

    @github-actions
    github-actionsbot
    on May 25, 2026 – with GitHub Actions
    Contributor

    This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 240 days).
    If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.

  16. github-actions
    closed this as not plannedon May 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    moduleIssues and PRs related to the module subsystem.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions