Skip to content

fix: harden check for non-module in vm.modules linker#47583

Closed
MikeRalphson wants to merge 2 commits intonodejs:mainfrom
MikeRalphson:patch-3
Closed

fix: harden check for non-module in vm.modules linker#47583
MikeRalphson wants to merge 2 commits intonodejs:mainfrom
MikeRalphson:patch-3

Conversation

@MikeRalphson
Copy link
Contributor

@MikeRalphson MikeRalphson commented Apr 16, 2023 •
edited
Loading 

(node:528088) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/vm/module:306
        if (module[kWrap] === undefined) {
                  ^

TypeError: Cannot read properties of undefined (reading 'Symbol(kWrap)')
    at ModuleWrap.<anonymous> (node:internal/vm/module:306:19)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Node.js v19.9.0

Repro case:

const vm = require('node:vm');

async function main() {
  const myObj = { myProp: '' };
  const context = vm.createContext(myObj);
  const module = new vm.SourceTextModule('import * as bug from "bug";', { identifier: 'bug', context });
  await module.link(function(a,b,c){return});
}

main();

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Apr 16, 2023
@RaisinTen
Copy link
Member

RaisinTen commented Apr 17, 2023

Should we add a test?

Also, cc @nodejs/vm.

@MikeRalphson
Copy link
Contributor Author

MikeRalphson commented Apr 17, 2023

@RaisinTen should that be a simple test of the exception type?

@RaisinTen
Copy link
Member

RaisinTen commented Apr 17, 2023

I think so, yes.

aduh95
aduh95 reviewed Apr 17, 2023
View reviewed changes
@MikeRalphson
Copy link
Contributor Author

MikeRalphson commented Apr 18, 2023

Have found the relevant test file and am working up the new test function.

@MikeRalphson
Copy link
Contributor Author

MikeRalphson commented Apr 19, 2023

Is there a way to run just one of, or a subset of, the tests?

@RaisinTen
Copy link
Member

RaisinTen commented Apr 19, 2023

Yea, you can run ./node test/parallel/test-rest-of-the-file-name.js to run an individual test

@MikeRalphson
Copy link
Contributor Author

MikeRalphson commented Apr 19, 2023

Thanks @RaisinTen!

@aduh95
Copy link
Contributor

aduh95 commented Apr 19, 2023

You can also use tools/test.py test/parallel/test-rest-of-the-file-name.js or e.g. tools/test.py test/parallel/test-repl-* if you wanted to run all the REPL tests.

@fhinkel
Copy link
Member

fhinkel commented Feb 3, 2024

@MikeRalphson are you still working on the regression test or can somebody else pick this up?

@legendecas legendecas mentioned this pull request Mar 20, 2024
Merged
@legendecas
Copy link
Member

legendecas commented Mar 22, 2024

Landed in d1d5da2.

@legendecas legendecas closed this Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments

@jasnell jasnell jasnell approved these changes

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@MikeRalphson @RaisinTen @aduh95 @fhinkel @legendecas @jasnell @nodejs-github-bot