fix: resolve cjs deps as cjs instead of esm #362

cb1kenobi · 2023-09-15T22:02:16Z

When nft analyzes a file that has both a require() and an ESM export, the first call to acorn fails because of the export, so it treats the whole file as ESM, which is probably fine.

The problem is it treats the require() as an ESM import which is wrong. This will cause the resolve dependency entrypoint in the dependency to get the ESM export instead of the proper CJS export.

If the dependency is an import, add it to imports. If it's a require(), add it to deps and don't worry about whatever isESM.

orca-security-us

Orca Security Scan Summary

Status	Check	Issues by priority
Passed	Secrets	0 0 0 0	View in Orca

styfle

Please add a test. Ideally an integration test with @vercel/edge-config but also a simple unit test would be good.

package.json

styfle

Looks good if tests pass, thanks!

styfle

Great, thanks!

github-actions · 2023-09-18T21:52:22Z

🎉 This PR is included in version 0.24.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

This reverts commit 1eba4d5.

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.

fix: resolve cjs deps as cjs instead of esm

e8f5e45

cb1kenobi requested review from ijjk and styfle as code owners September 15, 2023 22:02

orca-security-us bot reviewed Sep 15, 2023

View reviewed changes

styfle requested changes Sep 15, 2023

View reviewed changes

Use mixedModules and added a test

9ee7f46

styfle reviewed Sep 15, 2023

View reviewed changes

package.json Show resolved Hide resolved

Chris Barber and others added 2 commits September 16, 2023 04:47

Update lockfile

d0f51e1

Merge branch 'main' into resolve-cjs-as-cjs-not-esm

3ece299

styfle approved these changes Sep 16, 2023

View reviewed changes

Chris Barber added 2 commits September 18, 2023 15:57

Added test

6355382

Only enable mixedModules for edge config test

7e6466c

cb1kenobi requested a review from styfle September 18, 2023 21:36

styfle approved these changes Sep 18, 2023

View reviewed changes

styfle merged commit 1eba4d5 into main Sep 18, 2023
17 checks passed

styfle deleted the resolve-cjs-as-cjs-not-esm branch September 18, 2023 21:47

github-actions bot added the released label Sep 18, 2023

cb1kenobi mentioned this pull request Oct 2, 2023

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

Draft

cb1kenobi added a commit that referenced this pull request Oct 2, 2023

Revert "fix: resolve cjs deps as cjs instead of esm (#362)"

881b0e3

This reverts commit 1eba4d5.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resolve cjs deps as cjs instead of esm #362

fix: resolve cjs deps as cjs instead of esm #362

cb1kenobi commented Sep 15, 2023 •

edited by styfle

Loading

orca-security-us bot left a comment •

edited

Loading

styfle left a comment •

edited

Loading

styfle left a comment

styfle left a comment

github-actions bot commented Sep 18, 2023

fix: resolve cjs deps as cjs instead of esm #362

fix: resolve cjs deps as cjs instead of esm #362

Conversation

cb1kenobi commented Sep 15, 2023 • edited by styfle Loading

orca-security-us bot left a comment • edited Loading

Choose a reason for hiding this comment

Orca Security Scan Summary

styfle left a comment • edited Loading

Choose a reason for hiding this comment

styfle left a comment

Choose a reason for hiding this comment

styfle left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2023

cb1kenobi commented Sep 15, 2023 •

edited by styfle

Loading

orca-security-us bot left a comment •

edited

Loading

styfle left a comment •

edited

Loading