doc: mention existence/purpose of module wrapper#6433
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Fishrock123 commented Apr 27, 2016
Seems fine to me, but I might be missing some context. cc @nodejs/documentation |
cjihrig commented Apr 27, 2016
LGTM, but could you also update the statement that I referenced in #6427 (comment). |
Slayer95 commented Apr 28, 2016
Note that, as is, this opens the possibility that people -god forbid- start accessing |
mtharrisonforce-pushed the doc-module-wrapper branch from b9a9973 to 59523aeCompare
mtharrison commented Apr 28, 2016
@cjihrig sure, updated! |
mtharrisonforce-pushed the doc-module-wrapper branch from 59523ae to 6e9de4eCompare
doc/api/modules.md Outdated
cjihrig commented Apr 28, 2016
LGTM, but let's wait a day or so to let other collaborators weigh in. |
doc/api/modules.md Outdated
There was a problem hiding this comment.
thing? Wouldn't code be better?
evanlucas commented Apr 28, 2016
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! |
mtharrisonforce-pushed the doc-module-wrapper branch from f609afc to fbe510aComparemtharrison commented Apr 28, 2016
@evanlucas Thanks. I squashed into a single commit. Which looks like it conforms. Is it correct? |
doc/api/modules.md Outdated
There was a problem hiding this comment.
I wouldn’t reference V8 here directly, as this does not depend on the specific engine that’s being used.
There was a problem hiding this comment.
@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..."
There was a problem hiding this comment.
I like the change the whole sentence idea
evanlucas commented Apr 28, 2016
@mtharrison commit message looks good. Thanks! |
mtharrisonforce-pushed the doc-module-wrapper branch from fbe510a to be86e84Compare
doc/api/modules.md Outdated
There was a problem hiding this comment.
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
Left some comments. LGTM otherwise. |
Included a block in the modules.md file to explain the existence and purpose of the module wrapper.
mtharrisonforce-pushed the doc-module-wrapper branch from be86e84 to 696fc71Comparemtharrison commented May 3, 2016
@jasnell Thanks, I've addressed those issues now. |
jasnell commented May 3, 2016
Thank you! |
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 <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]>
evanlucas commented May 10, 2016
Landed in 7164003. Thanks! |
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 <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]>
11 participants
Checklist
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