Replace vague 'may not' with definitive 'will not' #23143

mikemaccana · 2018-09-28T11:04:08Z

This vagueness of 'may' has caused a great deal of confusion. See https://stackoverflow.com/questions/8887318/understanding-node-js-modules-multiple-requires-return-the-same-object

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

This vagueness of 'may' has caused a great deal of confusion. See https://stackoverflow.com/questions/8887318/understanding-node-js-modules-multiple-requires-return-the-same-object

vsemozhetbyt · 2018-09-28T15:42:15Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1050/

(Not sure why CI shows different commit though)

vsemozhetbyt · 2018-09-28T15:42:37Z

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

richardlau · 2018-09-28T15:51:10Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1050/

(Not sure why CI shows different commit though)

If you're referring to ea008c89204368d6534b1690a3fd99ed7976c9d2 (f4d400d02167747d4de3967589294fad1948fe31 is the commit for this PR) then it refers to the commit of the build repository that is used for the pipeline (we're storing the definition for the pipeline in the build repository, in this case https://github.com/nodejs/build/blob/ea008c89204368d6534b1690a3fd99ed7976c9d2/jenkins/pipelines/node-test-pull-request-lite-pipeline.jenkinsfile).

refack · 2018-09-28T20:45:01Z

/CC @nodejs/modules

jdalton · 2018-09-28T21:03:45Z

The may not is hinting at something that could be clarified in a more explicit way.
Which is that multiple calls to require('foo') will not cause the module code to be
executed multiple times unless it is removed from the module cache.

Fishrock123 · 2018-09-28T21:55:44Z

It could potentially be clarified that this will not happen under default circumstances. Once you modify the require cache all bets are off.

ljharb · 2018-09-28T22:24:18Z

doc/api/modules.md

@@ -200,7 +200,7 @@ Modules are cached after the first time they are loaded. This means
 (among other things) that every call to `require('foo')` will get
 exactly the same object returned, if it would resolve to the same file.

-Multiple calls to `require('foo')` may not cause the module code to be
+Multiple calls to `require('foo')` will not cause the module code to be


It’s “may” because someone could have deleted it from require.cache - i think “will” is incorrect.

mikemaccana

Updated as requested

mikemaccana

Typo.

mikemaccana · 2018-09-28T22:35:40Z

Updated per @Fishrock123 - now it's more specific:

By default (provided the require.cache is not modified), multiple calls to executed multiple times. This is an important feature. With it, require('foo') will not cause the module code to be executed multiple times.

Trott · 2018-09-28T23:04:03Z

doc/api/modules.md

-executed multiple times.  This is an important feature. With it,
-"partially done" objects can be returned, thus allowing transitive
-dependencies to be loaded even when they would cause cycles.
+By default (provided the `require.cache` is not modified), multiple calls to 


Optional nit: Get rid of By default and the parentheses:

Provided `require.cache` is not modified, multiple calls to

I'm fine with it the way it is but I think that change makes it a tiny bit more clear.

vsemozhetbyt · 2018-09-29T00:44:20Z

doc/api/modules.md

+By default (provided the `require.cache` is not modified), multiple calls to 
+`require('foo')` will not cause the module code to be executed multiple times.  
+
+This is an important feature. With it, "partially done" objects can be returned, 


This line has trailing space and exceeds the limit of 80 characters producing linter issue.

mcollina

LGTM with @Trott nit.

trivikr

LGTM.
Removing author ready label as there are some outstanding comments

Trott · 2018-09-30T16:41:34Z

Since the comments were rather minor (remove a space here, optionally change wording there), I did them rather than waiting for OP to do it and have restored the author ready label.

mikemaccana · 2018-09-30T17:40:04Z

Thanks Rich! I can enjoy the rest of my weekend 😀

…

On Sun, 30 Sep 2018 at 5:43 pm, Rich Trott ***@***.***> wrote: Since the comments were rather minor (remove a space here, optionally change wording there), I did them rather than waiting for OP to do it and have restored the author ready label. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#23143 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKiMh16ZcGm-8XxDsho6Kc6qNL8Ajh7ks5ugPS2gaJpZM4W-TUm> .

trivikr · 2018-10-01T15:21:52Z

CI Lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1091/

trivikr · 2018-10-01T16:26:18Z

Landed in a82ac6e

This vagueness of 'may' has caused a great deal of confusion. See https://stackoverflow.com/questions/8887318 PR-URL: #23143 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Replace vague 'may not' with definitive 'will not'

f4d400d

This vagueness of 'may' has caused a great deal of confusion. See https://stackoverflow.com/questions/8887318/understanding-node-js-modules-multiple-requires-return-the-same-object

nodejs-github-bot added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. labels Sep 28, 2018

addaleax approved these changes Sep 28, 2018

View reviewed changes

cjihrig approved these changes Sep 28, 2018

View reviewed changes

BridgeAR approved these changes Sep 28, 2018

View reviewed changes

vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 28, 2018

lpinca approved these changes Sep 28, 2018

View reviewed changes

ljharb reviewed Sep 28, 2018

View reviewed changes

Mention modules will load once by default.

cb473e7

mikemaccana commented Sep 28, 2018

View reviewed changes

Damn typo

43e4208

mikemaccana commented Sep 28, 2018

View reviewed changes

Trott reviewed Sep 28, 2018

View reviewed changes

jdalton approved these changes Sep 28, 2018

View reviewed changes

vsemozhetbyt reviewed Sep 29, 2018

View reviewed changes

xtuc approved these changes Sep 29, 2018

View reviewed changes

mcollina approved these changes Sep 29, 2018

View reviewed changes

benjamingr approved these changes Sep 29, 2018

View reviewed changes

thefourtheye approved these changes Sep 29, 2018

View reviewed changes

sendilkumarn approved these changes Sep 29, 2018

View reviewed changes

trivikr removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 30, 2018

trivikr approved these changes Sep 30, 2018

View reviewed changes

Trott added 3 commits September 30, 2018 09:37

Update modules.md

93b7750

Update modules.md

58e2fee

Update modules.md

f4bdf88

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 30, 2018

trivikr approved these changes Oct 1, 2018

View reviewed changes

trivikr closed this Oct 1, 2018

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace vague 'may not' with definitive 'will not' #23143

Replace vague 'may not' with definitive 'will not' #23143

mikemaccana commented Sep 28, 2018 •

edited

Loading

vsemozhetbyt commented Sep 28, 2018

vsemozhetbyt commented Sep 28, 2018

richardlau commented Sep 28, 2018

refack commented Sep 28, 2018

jdalton commented Sep 28, 2018

Fishrock123 commented Sep 28, 2018

ljharb Sep 28, 2018

mikemaccana left a comment

mikemaccana left a comment

mikemaccana commented Sep 28, 2018

Trott Sep 28, 2018

vsemozhetbyt Sep 29, 2018

mcollina left a comment

trivikr left a comment

Trott commented Sep 30, 2018

mikemaccana commented Sep 30, 2018 via email

trivikr commented Oct 1, 2018

trivikr commented Oct 1, 2018

Replace vague 'may not' with definitive 'will not' #23143

Replace vague 'may not' with definitive 'will not' #23143

Conversation

mikemaccana commented Sep 28, 2018 • edited Loading

Checklist

vsemozhetbyt commented Sep 28, 2018

vsemozhetbyt commented Sep 28, 2018

richardlau commented Sep 28, 2018

refack commented Sep 28, 2018

jdalton commented Sep 28, 2018

Fishrock123 commented Sep 28, 2018

ljharb Sep 28, 2018

Choose a reason for hiding this comment

mikemaccana left a comment

Choose a reason for hiding this comment

mikemaccana left a comment

Choose a reason for hiding this comment

mikemaccana commented Sep 28, 2018

Trott Sep 28, 2018

Choose a reason for hiding this comment

vsemozhetbyt Sep 29, 2018

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

trivikr left a comment

Choose a reason for hiding this comment

Trott commented Sep 30, 2018

mikemaccana commented Sep 30, 2018 via email

trivikr commented Oct 1, 2018

trivikr commented Oct 1, 2018

mikemaccana commented Sep 28, 2018 •

edited

Loading