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

doc: fix, unify, formalize, and amplify vm.md #25422

Closed
wants to merge 2 commits into from

Conversation

@vsemozhetbyt
Copy link
Contributor

commented Jan 9, 2019

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

vm module API heavily reuses common code, but the doc seems to be a bit out of date: some options are listed in wrong places, some options and history entries are missed.

Also, some fragments need to be formalized and unified.

doc: fix, unify, formalize, and amplify vm.md
`vm` module API heavily reuses common code, but the doc seems
to be a bit out of date: some options are listed in wrong places,
some options and history entries are missed.

Also some fragments need to be formalized and unified.
@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

cc @nodejs/documentation @nodejs/vm

@vsemozhetbyt vsemozhetbyt added the vm label Jan 9, 2019

@devsnek
devsnek approved these changes Jan 9, 2019
@jdalton

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

Pulling in @guybedford from #25421 (comment). He mentioned the importModuleDynamically could be considered experimental. I don't see it documented as such and I've heard mixed messaging on it so I think we should clarify.

It also looks like importModuleDynamically doesn't work without --experimental-modules to enable experimental ESM support so that should probably be documented as well.

@vsemozhetbyt

This comment was marked as resolved.

Copy link
Contributor Author

commented Jan 10, 2019

I am not sure how exactly this should be documented. Could anybody push a commit in this PR? Or create a new one.

* Returns: {Module Namespace Object|vm.SourceTextModule} Returning a
`vm.SourceTextModule` is recommended in order to take advantage of error
tracking, and to avoid issues with namespaces that contain `then`
function exports.

This comment was marked as resolved.

Copy link
@guybedford

guybedford Jan 10, 2019

Contributor

I would suggest including a note here saying something like:

"This option is part of the experimental API for the --experimental-modules flag, and should not be considered stable."

This comment has been minimized.

Copy link
@guybedford

guybedford Jan 10, 2019

Contributor

Sorry I copied the wrong one, this should apply to the whole vm.sourceTextModule section.

The note above should apply only for the vm.Script option.

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jan 10, 2019

Author Contributor

Added in vm.Script() constructor and all concerned vm.runIn...() methods.

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jan 10, 2019

Author Contributor

vm.sourceTextModule already has experimental note.

* Returns: {Module Namespace Object|vm.SourceTextModule} Returning a
`vm.SourceTextModule` is recommended in order to take advantage of error
tracking, and to avoid issues with namespaces that contain `then`
function exports.

This comment was marked as resolved.

Copy link
@guybedford

guybedford Jan 10, 2019

Contributor

"This option is part of the experimental API for the --experimental-modules flag, and should not be considered stable."

To go here.

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

The note is added to all concerned places in a new commit.

@vsemozhetbyt vsemozhetbyt force-pushed the vsemozhetbyt:doc-vm-formalize branch from d5c8cb9 to 2f837fa Jan 10, 2019

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

Landed in c69ea3b
Thank you for the reviews.

@vsemozhetbyt vsemozhetbyt deleted the vsemozhetbyt:doc-vm-formalize branch Jan 11, 2019

vsemozhetbyt added a commit that referenced this pull request Jan 11, 2019
doc: fix, unify, formalize, and amplify vm.md
`vm` module API heavily reuses common code, but the doc seems
to be a bit out of date: some options are listed in wrong places,
some options and history entries are missed.

Also some fragments need to be formalized and unified.

PR-URL: #25422
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
addaleax added a commit that referenced this pull request Jan 14, 2019
doc: fix, unify, formalize, and amplify vm.md
`vm` module API heavily reuses common code, but the doc seems
to be a bit out of date: some options are listed in wrong places,
some options and history entries are missed.

Also some fragments need to be formalized and unified.

PR-URL: #25422
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
doc: fix, unify, formalize, and amplify vm.md
`vm` module API heavily reuses common code, but the doc seems
to be a bit out of date: some options are listed in wrong places,
some options and history entries are missed.

Also some fragments need to be formalized and unified.

PR-URL: nodejs#25422
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.