fix dynamic require of node:-namespaced builtins #2755
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
boneskull
force-pushed
the
boneskull/fix-dynamic-builtin-2754
branch
from
93c3756 to
133bc0e
Compare
boneskull
commented
Apr 3, 2025
boneskull
commented
Apr 3, 2025
boneskull
added a commit
to LavaMoat/LavaMoat
that referenced
this pull request
Apr 5, 2025
- add failing test for dynamic require of node:-namespaced builtin modules (see endojs/endo#2755)
boneskull
force-pushed
the
boneskull/fix-dynamic-builtin-2754
branch
from
133bc0e to
24b48c4
Compare
boneskull
added a commit
to LavaMoat/LavaMoat
that referenced
this pull request
Apr 14, 2025
- add failing test for dynamic require of node:-namespaced builtin modules (see endojs/endo#2755)
kriskowal
approved these changes
Apr 14, 2025
|
|
||
| const builtin = 'node:cluster'; | ||
|
|
||
| require(builtin); |
boneskull
force-pushed
the
boneskull/fix-dynamic-builtin-2754
branch
2 times, most recently
from
6413cdb to
2f244b1
Compare
I saw this in a package somewhere; it is plainly invalid. But it would throw an exception unless we ignore it.
It will now return `AsyncParseFn` xor `SyncParseFn` instead of a union of the two. The function now requires a boolean value for `preferSynchronous`. This is an internal function, so this isn't a breaking change. Type arguments are unfriendly to default parameter values, which is why `= false` needed removal.
closes #2754 Dynamically-required builtins were going through the entire candidate-specifier-guess-'n'-check loop, which shouldn't happen. Specifically, the call to a `MaybeReadNowFn` in this loop caused the original error as reported in #2754 because it uses a `FileUrlToPathFn`; the default implementation (Node.js' `url.fileURLToPath`) throws when provided `node:foo` _but does not throw_ when provided `foo`. Also exports `WILDCARD_POLICY_VALUE` from `policy-format` so we can use it instead of magic strings all over the place. Fixes #2754
boneskull
force-pushed
the
boneskull/fix-dynamic-builtin-2754
branch
from
2f244b1 to
df973b2
Compare
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #2754
Description
This fixes a couple of bugs, actually:
inferExportsno longer throws when encountering the plainly-invalid-but-first-discovered-in-the-wild export alias of./. This alias is now ignored.importNowHooknow handles dynamically-required builtins without considering them to first be packages, which a) is faster and b) avoids a potential thrown exception when aFileUrlToPathFnencounters a module specifier that appears to have a scheme but is not thefile:scheme (likenode:fs). This was the root cause of the issue in compartment-mapper: dynamic require of "node:"-prefixed builtins fails #2754.And some tangents:
inferExportsissue, I refactored type signatures to usePackageDescriptorinstead ofobject. This required defining a few more fields inPackageDescriptorwhichinfer-exports.jsconsiders.WILDCARD_POLICY_VALUEis now interally-exported frompolicy-format.jswhich should probably be used instead of'any'in the context of policy (wherever possible). It's now used in the test suite for dynamic requires.And with shame:
mapParsersto have the signature that I had originally intended. And I didn't feel like it deserved an entire PR. This is completely unrelated to anything else hereSecurity Considerations
n/a
Scaling Considerations
It should be faster for
@endo/compartment-mapperto crunch dynamically-required builtins.Documentation Considerations
I don't think this needs any documentation changes.
Testing Considerations
I've added coverage for #2754, but did not add coverage for anything else. I should probably add coverage for the change to
inferExports.Compatibility Considerations
Backwards compatible
Upgrade Considerations