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

add Node.js v20 support #28

Merged
merged 7 commits into from
Jun 2, 2023
Merged

add Node.js v20 support #28

merged 7 commits into from
Jun 2, 2023

Conversation

bengl
Copy link
Member

@bengl bengl commented Jun 2, 2023

Since Node.js v20 moves loaders to a separate thread, we can no longer depend on loading the modules in the loader to get the exports. This is needed to add our mutable proxy. For Node.js 20, exports are now retrieved via parsing.

To reduce startup overhead on older versions of Node.js, the previous method of getting exports is used, avoiding loading and using parsers.

bengl added 5 commits June 2, 2023 00:55
Since Node.js v20 moves loaders to a separate thread, we can no longer
depend on loading the modules in the loader to get the exports. This is
needed to add our mutable proxy. For Node.js 20, exports are now
retrieved via parsing.

To reduce startup overhead on older versions of Node.js, the previous
method of getting exports is used, avoiding loading and using parsers.
lib/get-esm-exports.js Outdated Show resolved Hide resolved
lib/get-exports.js Outdated Show resolved Hide resolved
@@ -37,6 +36,9 @@
"typescript": "^4.7.4"
},
"dependencies": {
"acorn": "^8.8.2",
Copy link
Contributor

@tlhunter tlhunter Jun 2, 2023

Choose a reason for hiding this comment

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

I don't see any output on the PR about the module sizes...

For a naked project, installing these two packages results in 528K of disk space. Doesn't look like any of that would be deduped with existing dependencies since acorn itself has no deps:

λ /tmp/acorn/ npm ls --depth=10
[email protected] /private/tmp/acorn
├─┬ [email protected]
│ └── [email protected] deduped
└── [email protected]

Copy link
Contributor

Choose a reason for hiding this comment

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

Compressed, the files take up 133KB, which would be the approx change to a Lambda bundle.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clever stuff

Copy link
Contributor

@tlhunter tlhunter left a comment

Choose a reason for hiding this comment

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

Overall this looks pretty good but we should have @Qard or @rochdev weigh in first.

Copy link
Member

@Qard Qard left a comment

Choose a reason for hiding this comment

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

Some questions for you, but otherwise LGTM.

.github/workflows/ci.yml Show resolved Hide resolved
hook.js Outdated Show resolved Hide resolved
lib/get-exports.js Outdated Show resolved Hide resolved
}

// At this point our `format` is either undefined or not known by us. Fall
// back to parsing as ESM/CJS.
Copy link
Member

Choose a reason for hiding this comment

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

Could it not be wasm? What happens in that case?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK that's not in Node.js 20, so we'll cross that bridge when we come to it.

Copy link
Member

Choose a reason for hiding this comment

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

It's behind a flag, but it is there. There's also json modules.

If I understand right, if we don't return anything from the loader then it will skip it and use the default/built-in logic, correct? So just not handling formats we don't specifically support here is fine?

lib/get-esm-exports.js Outdated Show resolved Hide resolved
test/fixtures/esm-exports.txt Show resolved Hide resolved
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.

3 participants