FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

vm: rename vm.Module to vm.SourceTextModule by devsnek · Pull Request #22007 · nodejs/node · GitHub

/ node Public

vm: rename vm.Module to vm.SourceTextModule - #22007

Merged
devsnek merged 1 commit into
nodejs:masterfrom
devsnek:rename-vm-module
Jul 31, 2018
ConversationCommits (1)ChecksFiles changed
Merged

vm: rename vm.Module to vm.SourceTextModule#22007
devsnek merged 1 commit into
nodejs:masterfrom
devsnek:rename-vm-module

Conversation

devsnek commented Jul 28, 2018

Copy link
Copy Markdown
Member

Should have always been named SourceTextModule

/cc @nodejs/vm

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

devsnek added the vm Issues and PRs related to the vm subsystem. label Jul 28, 2018

nodejs-github-bot commented Jul 28, 2018

Copy link
Copy Markdown
Collaborator

@devsnek build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/369/pipeline

nodejs-github-bot added build Issues and PRs related to build files or the CI. process Issues and PRs related to the process subsystem. vm Issues and PRs related to the vm subsystem. labels Jul 28, 2018

devsnek commented Jul 28, 2018

Copy link
Copy Markdown
Member Author

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

targos commented Jul 28, 2018

Copy link
Copy Markdown
Member

Is it in preparation for adding other kinds of modules?

devsnek commented Jul 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

@targos mostly... even if the spec doesn't define any more types we should still name them correctly.

cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 28, 2018
cjihrig approved these changes Jul 28, 2018
View reviewed changes

cjihrig 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No opinion on the change, but the code LGTM.

targos commented Jul 28, 2018

Copy link
Copy Markdown
Member

It doesn't have to be semver major. The API is experimental and behind a flag

TimothyGu removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 28, 2018

addaleax commented Jul 28, 2018

Copy link
Copy Markdown
Member

Not that you have to, but can you explain this? What kind of ES modules would not have some kind of source text associated with them? If this is to distinguish from e.g. WASM modules or so, maybe referring to the used language might be better?

(Also, fyi: @nodejs/modules)

TimothyGu commented Jul 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

@addaleax At the last TC39 meeting, a new type of Module Records backed by JavaScript source called Dynamic Module Records was discussed, and it is now at Stage 1. Regardless of whether that proposal makes it all the way into the spec, SourceTextModule is indeed a more descriptive and accurate name for what this class represents.

TimothyGu commented Jul 29, 2018

Copy link
Copy Markdown
Member

@devsnek Would also be nice if my last comment (#22007 (comment)) is integrated into the commit message body.

devsnek commented Jul 29, 2018

Copy link
Copy Markdown
Member Author

@TimothyGu will do when I land 👍

mcollina approved these changes Jul 29, 2018
View reviewed changes

mcollina left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

targos commented Jul 29, 2018

Copy link
Copy Markdown
Member

Resumed: https://ci.nodejs.org/job/node-test-pull-request/16057/

vm: rename vm.Module to vm.SourceTextModule
2bdcdfc 
At the last TC39 meeting, a new type of Module Records backed by
JavaScript source called Dynamic Module Records was discussed, and
it is now at Stage 1. Regardless of whether that proposal makes it
all the way into the spec, SourceTextModule is indeed a more
descriptive and accurate name for what this class represents.

PR-URL: nodejs#22007
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
devsnek force-pushed the rename-vm-module branch from 62adcd3 to 2bdcdfc Compare July 31, 2018 15:24
devsnek closed this Jul 31, 2018
devsnek merged commit 2bdcdfc into nodejs:master Jul 31, 2018
devsnek deleted the rename-vm-module branch July 31, 2018 15:25

devsnek commented Jul 31, 2018

Copy link
Copy Markdown
Member Author

landed in 2bdcdfc

targos pushed a commit that referenced this pull request Aug 1, 2018
vm: rename vm.Module to vm.SourceTextModule
9a88fe4 
At the last TC39 meeting, a new type of Module Records backed by
JavaScript source called Dynamic Module Records was discussed, and
it is now at Stage 1. Regardless of whether that proposal makes it
all the way into the spec, SourceTextModule is indeed a more
descriptive and accurate name for what this class represents.

PR-URL: #22007
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
targos mentioned this pull request Aug 1, 2018
Merged
This was referenced Aug 1, 2018
Closed
Closed
Closed
Closed
Closed
Closed
refack mentioned this pull request Oct 18, 2018
Merged
3 tasks
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ljharb ljharb approved these changes
mcollina mcollina approved these changes
jasnell jasnell approved these changes
hybrist hybrist approved these changes
benjamingr benjamingr approved these changes
cjihrig cjihrig approved these changes
BridgeAR BridgeAR approved these changes
+5 more reviewers
jdalton jdalton approved these changes
bmeck bmeck approved these changes
TimothyGu TimothyGu approved these changes
targos targos approved these changes
hiroppy hiroppy approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. process Issues and PRs related to the process subsystem. 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.

Back | FazBrowse Home | New Git URL