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

doc: mention existence/purpose of module wrapper by mtharrison · Pull Request #6433 · nodejs/node · GitHub

/ node Public

doc: mention existence/purpose of module wrapper - #6433

Closed
mtharrison wants to merge 1 commit into
nodejs:masterfrom
mtharrison:doc-module-wrapper
ConversationCommits (1)ChecksFiles changed
Closed

doc: mention existence/purpose of module wrapper#6433
mtharrison wants to merge 1 commit into
nodejs:masterfrom
mtharrison:doc-module-wrapper

Conversation

mtharrison commented Apr 27, 2016

Copy link
Copy Markdown
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 added the doc Issues and PRs related to the documentations. label Apr 27, 2016
mtharrison mentioned this pull request Apr 27, 2016
Closed

Fishrock123 commented Apr 27, 2016

Copy link
Copy Markdown
Contributor

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

cjihrig commented Apr 27, 2016

Copy link
Copy Markdown
Contributor

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

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

Slayer95 commented Apr 28, 2016

Copy link
Copy Markdown
Contributor

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

mtharrison commented Apr 28, 2016

Copy link
Copy Markdown
Contributor Author

@cjihrig sure, updated!

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

cjihrig Apr 28, 2016

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

Extra space after "to"

mtharrison Apr 28, 2016

Copy link
Copy Markdown
Contributor Author

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

Fixed.

cjihrig commented Apr 28, 2016

Copy link
Copy Markdown
Contributor

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

thefourtheye reviewed Apr 28, 2016
View reviewed changes
Comment thread doc/api/modules.md Outdated

thefourtheye Apr 28, 2016

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

thing? Wouldn't code be better?

mtharrison Apr 28, 2016

Copy link
Copy Markdown
Contributor Author

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

Sorted.

evanlucas commented Apr 28, 2016

Copy link
Copy Markdown
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 commented Apr 28, 2016

Copy link
Copy Markdown
Contributor Author

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

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

addaleax Apr 28, 2016

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

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

evanlucas Apr 28, 2016

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

ah good point @addaleax

mtharrison Apr 28, 2016

Copy link
Copy Markdown
Contributor Author

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

@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..."

evanlucas Apr 28, 2016

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

I like the change the whole sentence idea

mtharrison Apr 28, 2016

Copy link
Copy Markdown
Contributor Author

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

@evanlucas thanks, updated accordingly.

evanlucas commented Apr 28, 2016

Copy link
Copy Markdown
Contributor

@mtharrison commit message looks good. Thanks!

jasnell reviewed Apr 29, 2016
View reviewed changes
Comment thread doc/api/modules.md Outdated

jasnell Apr 29, 2016

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

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 commented Apr 29, 2016

Copy link
Copy Markdown
Member

Left some comments. LGTM otherwise.

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 force-pushed the doc-module-wrapper branch from be86e84 to 696fc71 Compare May 3, 2016 18:26

mtharrison commented May 3, 2016

Copy link
Copy Markdown
Contributor Author

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

jasnell commented May 3, 2016

Copy link
Copy Markdown
Member

Thank you!

evanlucas pushed a commit that referenced this pull request May 10, 2016
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 commented May 10, 2016

Copy link
Copy Markdown
Contributor

Landed in 7164003. Thanks!

evanlucas closed this May 10, 2016
mtharrison deleted the doc-module-wrapper branch May 10, 2016 11:52
evanlucas pushed a commit that referenced this pull request May 17, 2016
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 mentioned this pull request May 17, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
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
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 mentioned this pull request Jun 24, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
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
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>
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

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.

Back | FazBrowse Home | New Git URL