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

esm loaders: getBuiltin() does not work with node:test #48516

Closed
cjihrig opened this issue Jun 21, 2023 · 2 comments · Fixed by #48779
Closed

esm loaders: getBuiltin() does not work with node:test #48516

cjihrig opened this issue Jun 21, 2023 · 2 comments · Fixed by #48779
Labels
loaders Issues and PRs related to ES module loaders

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Jun 21, 2023

Version

latest main

Platform

all

Subsystem

esm, loaders

What steps will reproduce the bug?

Create an ESM loader, and in the globalPreload() hook, use getBuiltin() to access node:test (test without the node: scheme also does not work).

How often does it reproduce? Is there a required condition?

Always reproduces.

What is the expected behavior? Why is that the expected behavior?

node:test can be used with getBuiltin()

What do you see instead?

TypeError [ERR_INVALID_ARG_VALUE]: The argument 'builtinName' is invalid. Received 'node:test'

Additional information

I imagine this is related to the use of BuiltinModule.canBeRequiredWithoutScheme(builtinName) in getBuiltin().

My use case is to attach the message port to the node:test module. For now, I have worked around this by attaching the port to globalThis instead. Once nodejs/loaders#147 is resolved, I won't need to do this, but I wanted to raise the issue in case the loaders team thinks it's important.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Jun 22, 2023

I assume that it would probably be trivial to add support for the node: prefix to getBuiltin(), assuming that fixes this, if you want to spend the time. But yeah, the whole sloppy-mode script that seems like CommonJS but isn’t really . . . this is what we’re planning on removing, because of issues like this.

cc @nodejs/loaders

@GeoffreyBooth GeoffreyBooth added the loaders Issues and PRs related to ES module loaders label Jun 22, 2023
@JakobJingleheimer
Copy link
Member

I was just looking at that util a couple days ago. It reads/validates from a list; test is probably just missing from that list. I don't know why it wouldn't be included—oversight or deliberate decision.

I expect register (including the message stuff) to land around mid-July.

aduh95 added a commit that referenced this issue Jul 17, 2023
PR-URL: #48779
Fixes: #48778
Fixes: #48516
Refs: #46402
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
pluris pushed a commit to pluris/node that referenced this issue Aug 6, 2023
pluris pushed a commit to pluris/node that referenced this issue Aug 7, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
UlisesGascon pushed a commit to UlisesGascon/node that referenced this issue Aug 14, 2023
RafaelGSS pushed a commit that referenced this issue Aug 15, 2023
PR-URL: #48779
Fixes: #48778
Fixes: #48516
Refs: #46402
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
targos pushed a commit to targos/node that referenced this issue Nov 11, 2023
targos pushed a commit that referenced this issue Nov 23, 2023
PR-URL: #48779
Backport-PR-URL: #50669
Fixes: #48778
Fixes: #48516
Refs: #46402
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loaders Issues and PRs related to ES module loaders
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants