Skip to content

vm: SourceTextModule refactor - #29776

Closed
devsnek wants to merge 1 commit into
nodejs:masterfrom
devsnek:vm-module-2
Closed

vm: SourceTextModule refactor#29776
devsnek wants to merge 1 commit into
nodejs:masterfrom
devsnek:vm-module-2

Conversation

@devsnek

@devsnekdevsnek commented Sep 30, 2019

Copy link
Copy Markdown
Member
  • Removes redundant instantiate method
  • Refactors link to match the spec linking steps more accurately
  • Removes URL validation from SourceTextModule specifiers
  • DRYs some dynamic import logic

Closes#29030

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@devsnekdevsnek added the vm Issues and PRs related to the vm subsystem. label Sep 30, 2019
@nodejs-github-botnodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 30, 2019
@devsnek

devsnek commented Sep 30, 2019

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/vm

@devsnekdevsnek removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 30, 2019
targos
targos reviewed Sep 30, 2019
View reviewed changes
Comment threaddoc/api/vm.md Outdated
Comment threadlib/internal/vm/source_text_module.js Outdated
@devsnekdevsnek added the experimental Issues and PRs related to experimental features. label Sep 30, 2019
@nodejs-github-bot

nodejs-github-bot commented Sep 30, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25769/

guybedford
guybedford reviewed Sep 30, 2019
View reviewed changes

@guybedfordguybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great to see these refinements.

Comment threaddoc/api/vm.md Outdated
Comment threadlib/internal/vm/source_text_module.js Outdated
BridgeAR
BridgeAR reviewed Oct 1, 2019
View reviewed changes
Comment threadlib/internal/errors.js Outdated
Comment threadlib/internal/vm/source_text_module.js Outdated

@BridgeARBridgeAROct 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest to abstract this into a private function for less repetition. That way it could be used as: this.#getPrivateProperty(name) where name would e.g., be '#error'.

@devsnekdevsnekOct 1, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

obj['#error'] is not the same as obj.#error, also you have to check for this.#getPrivateProperty in the first place 🙃

@BridgeARBridgeAROct 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Uh, of course! Too bad that it seems like there's no nice way around this 😞

@BridgeARBridgeAROct 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's pretty ugly but it should be possible to abstract it like this:

functiongetPrivateProperty(module,name){try{switch(name){case'error':
returnmodule.#error;
...
}}catch{thrownewERR_VM_MODULE_NOT_MODULE();}}

@devsnekdevsnekOct 1, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think how the current code is the best we can get it. you can blame tc39 for not generalizing private fields better 🤷

@devsnek@targos
vm: refactor SourceTextModule
c002456
- Removes redundant `instantiate` method
- Refactors `link` to match the spec linking steps more accurately
- Removes URL validation from SourceTextModule specifiers
- DRYs some dynamic import logic
Closesnodejs#29030
Co-Authored-By: Michaël Zasso <targos@protonmail.com>
@nodejs-github-bot

nodejs-github-bot commented Oct 1, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25809/

@devsnekdevsnek added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 2, 2019
Trott pushed a commit that referenced this pull request Oct 2, 2019
@devsnek@Trott
vm: refactor SourceTextModule
5e7946f
- Removes redundant `instantiate` method
- Refactors `link` to match the spec linking steps more accurately
- Removes URL validation from SourceTextModule specifiers
- DRYs some dynamic import logic
Closes: #29030
Co-Authored-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #29776
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@Trott

Trott commented Oct 2, 2019

Copy link
Copy Markdown
Member

Landed in 5e7946f

@TrottTrott closed this Oct 2, 2019
@devsnek
devsnek deleted the vm-module-2 branch October 2, 2019 23:50
BridgeAR pushed a commit that referenced this pull request Oct 9, 2019
@devsnek@BridgeAR
vm: refactor SourceTextModule
6d88f0f
- Removes redundant `instantiate` method
- Refactors `link` to match the spec linking steps more accurately
- Removes URL validation from SourceTextModule specifiers
- DRYs some dynamic import logic
Closes: #29030
Co-Authored-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #29776
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@BridgeARBridgeAR mentioned this pull request Oct 10, 2019
Merged
@chenjiahanchenjiahan mentioned this pull request Aug 2, 2025
Merged
2 tasks
@joyeecheungjoyeecheung mentioned this pull request Sep 14, 2025
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guybedfordguybedfordguybedford left review comments
@targostargostargos approved these changes
@BridgeARBridgeARBridgeAR left review comments
@jasnelljasnelljasnell approved these changes
@JungMinuJungMinuJungMinu approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.experimentalIssues and PRs related to experimental features.vmIssues and PRs related to the vm subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

docs vm.SourceTextModule: Instantiate() abstract operation has been replaced with InitializeEnvironment()

8 participants

@devsnek@nodejs-github-bot@Trott@jasnell@guybedford@targos@JungMinu@BridgeAR