8000 module: disallow CJS <-> ESM edges in a cycle from require(esm) by joyeecheung · Pull Request #52264 · nodejs/node · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@joyeecheung
Copy link
Member
@joyeecheung joyeecheung commented Mar 29, 2024

This patch disallows CJS <-> ESM cycle edges when they come from require(esm) requested in ESM evalaution.

Drive-by: don't reuse the cache for imported CJS modules to stash source code of required ESM because the former is also used for cycle detection.

Fixes: #52145

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/vm

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 29, 2024
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 29, 2024
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 31, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 31, 2024
@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor

Just to confirm - can this deal with the case where the require(esm) module is to a parent of a cycle that hasn't been executed yet? That is, the invariant should specifically be that none of the require(esm) module, nor any of its transitive dependencies are currently part of an ESM execution operation.

Consider the graph:

A (esm) -> B (esm)
B (esm) -> C (cjs)
C (cjs) -> Z (esm)
Z (esm) -> A (esm)

Where we import A first, and when the require(esm) gets Z it will be linked but not previously evaluated.

That is, my intuition here would be that a full upfront graph analysis must be done at require(esm) time to ensure acyclic behaviour.

This patch disallows CJS <-> ESM edges when they come from
require(esm) requested in ESM evalaution.

Drive-by: don't reuse the cache for imported CJS modules to stash
source code of required ESM because the former is also used for
cycle detection.
@joyeecheung
Copy link
Member Author

Where we import A first, and when the require(esm) gets Z it will be linked but not previously evaluated.

Yes this can be detected. Added a test.