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.

Sign up for GitHub

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

Jump to bottom

doc: mention existence/purpose of module wrapper #6433

Closed
wants to merge 1 commit into from
Closed

doc: mention existence/purpose of module wrapper #6433

wants to merge 1 commit into from

Conversation

mtharrison
Copy link
Contributor

Checklist
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Included a block in the modules.md file to explain the existence and
purpose of the module wrapper

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 27, 2016
@mtharrison mtharrison mentioned this pull request Apr 27, 2016
Closed
@Fishrock123
Copy link
Contributor

Seems fine to me, but I might be missing some context. cc @nodejs/documentation

@cjihrig
Copy link
Contributor

cjihrig commented Apr 27, 2016

LGTM, but could you also update the statement that I referenced in #6427 (comment).

@mscdex mscdex added the module Issues and PRs related to the module subsystem. label Apr 28, 2016
@Slayer95
Copy link

Note that, as is, this opens the possibility that people -god forbid- start accessing arguments from modules...

@mtharrison
Copy link
Contributor Author

@cjihrig sure, updated!

cjihrig
cjihrig reviewed Apr 28, 2016
View reviewed changes
doc/api/modules.md

- It keeps top-level variables (defined with `var`, `const` or `let`) scoped to
your module rather than the global object
- It helps to provide some global-looking variables that are actually specific
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra space after "to"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 28, 2016

LGTM, but let's wait a day or so to let other collaborators weigh in.

thefourtheye
thefourtheye reviewed Apr 28, 2016
View reviewed changes
doc/api/modules.md
<!-- type=misc -->

Before Node.js hands off your module to be executed by V8, it wraps the entire
thing in a function wrapper that looks like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

thing? Wouldn't code be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorted.

@evanlucas
Copy link
Contributor

LGTM with mine and @thefourtheye's nits. Thanks for the PR @mtharrison!

Also, can you update the commit title and message to conform to https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit? Specifically, the title and body line lengths. Thanks!

@mtharrison
Copy link
Contributor Author

@evanlucas Thanks. I squashed into a single commit. Which looks like it conforms. Is it correct?

addaleax
addaleax reviewed Apr 28, 2016
View reviewed changes
doc/api/modules.md

<!-- type=misc -->

Before Node.js hands off your module to be executed by V8, it wraps your code
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn’t reference V8 here directly, as this does not depend on the specific engine that’s being used.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah good point @addaleax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax @evanlucas Do you have any suggestions for something more appropriate? Some ideas:

  • s/V8/the JavaScript [runtime|engine]
  • Change the whole sentence to "Before Node.js executes the code in your module, it wraps it in..."

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the change the whole sentence idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evanlucas thanks, updated accordingly.

@evanlucas
Copy link
Contributor

@mtharrison commit message looks good. Thanks!

jasnell
jasnell reviewed Apr 29, 2016
View reviewed changes
doc/api/modules.md
<!-- type=misc -->

Before Node.js executes the code in your module, it wraps it with a function
wrapper that looks like this:
Copy link
Member

Choose a reason for hiding this comment

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

I know this particular document is riddled with you and your, which we should get refactored out later.. can you reword this to avoid adding a new instance of your? Perhaps, Before a module's code is executed, Node.js will wrap it with a function wrapper.

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

Left some comments. LGTM otherwise.

@mtharrison
doc: mention existence/purpose of module wrapper
696fc71 
Included a block in the modules.md file to explain the existence and
purpose of the module wrapper.
@mtharrison mtharrison force-pushed the doc-module-wrapper branch from be86e84 to 696fc71 Compare May 3, 2016 18:26
@mtharrison
Copy link
Contributor Author

@jasnell Thanks, I've addressed those issues now.

@jasnell
Copy link
Member

jasnell commented May 3, 2016

Thank you!

evanlucas pushed a commit that referenced this pull request May 10, 2016
@mtharrison @evanlucas
doc: mention existence/purpose of module wrapper
7164003 
Included a block in the modules.md file to explain the existence and
purpose of the module wrapper.

PR-URL: #6433
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas
Copy link
Contributor

Landed in 7164003. Thanks!

@evanlucas evanlucas closed this May 10, 2016
@mtharrison mtharrison deleted the doc-module-wrapper branch May 10, 2016 11:52
evanlucas pushed a commit that referenced this pull request May 17, 2016
@mtharrison @evanlucas
doc: mention existence/purpose of module wrapper
d34343f 
Included a block in the modules.md file to explain the existence and
purpose of the module wrapper.

PR-URL: #6433
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas evanlucas mentioned this pull request May 17, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
@mtharrison
doc: mention existence/purpose of module wrapper
7fdfcd3 
Included a block in the modules.md file to explain the existence and
purpose of the module wrapper.

PR-URL: #6433
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
@mtharrison
doc: mention existence/purpose of module wrapper
b786a0c 
Included a block in the modules.md file to explain the existence and
purpose of the module wrapper.

PR-URL: #6433
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jun 24, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
@mtharrison
doc: mention existence/purpose of module wrapper
ac6b921 
Included a block in the modules.md file to explain the existence and
purpose of the module wrapper.

PR-URL: #6433
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
@mtharrison
doc: mention existence/purpose of module wrapper
1da4651 
Included a block in the modules.md file to explain the existence and
purpose of the module wrapper.

PR-URL: #6433
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.