Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

demonstrate mjs file identified as esm, but handled as cjs #364

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cb1kenobi
Copy link
Contributor

@cb1kenobi cb1kenobi commented Oct 2, 2023

When a .mjs file is traced, it adds all import into deps instead of imports. Later in the tracing, it treats deps as CJS and it incorrectly resolves the default export for the playwright-core package.

This regression was introduced by #362, but that PR does fix the issue of CJS files being resolved incorrectly.

I don't know the background, but it seems all imports, especially in .mjs files, should go into imports, not deps.

@styfle can you please advise?

Test failure:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/private/var/folders/tr/t1h0gs514fl543bw43pchxpw0000gn/T/node-file-trace-0564114059456724/node_modules/playwright-core/index.mjs' imported from /private/var/folders/tr/t1h0gs514fl543bw43pchxpw0000gn/T/node-file-trace-0564114059456724/test/integration/playwright-core-esm.mjs
Did you mean to import playwright-core/index.js?
    at new NodeError (node:internal/errors:387:5)
    at finalizeResolution (node:internal/modules/esm/resolve:330:11)
    at moduleResolve (node:internal/modules/esm/resolve:907:10)
    at defaultResolve (node:internal/modules/esm/resolve:1115:11)
    at nextResolve (node:internal/modules/esm/loader:163:28)
    at ESMLoader.resolve (node:internal/modules/esm/loader:837:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:424:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:76:40)
    at link (node:internal/modules/esm/module_job:75:36) {

Copy link

@orca-security-us orca-security-us bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca

@styfle
Copy link
Member

styfle commented Oct 2, 2023

@cb1kenobi This test is passing https://github.com/vercel/nft/actions/runs/6384547761/job/17327429573?pr=364#step:8:6107

Is that because it doesn't set mixedModules: true?

@cb1kenobi
Copy link
Contributor Author

@styfle Correct. I didn't commit the change to integration.test.js that enables mixedModules = true for this test.

styfle pushed a commit that referenced this pull request Oct 2, 2023
This reverts commit 1eba4d5.

The fix in the original PR did solve the problem of tracing a dependency
being loaded from an ESM file via `require()`, but also caused a
regression where a simple `.mjs` file with ESM imports was failing. I
have a test (#364) for that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants