doc: improve module.builtinModules documentation #17712

watson · 2017-12-16T17:52:15Z

In this PR I've kept the original casing of the object module, but I think it might be easier to understand if we changed the entire section to use the uppercase version: Module. This have two benefits:

It matches what's actually exported when requiring "module"
It's easier to distinguish from the other module object that's available by default to all file modules

That change would instead look like this:

-### module.builtinModules
+### Module.builtinModules
 <!-- YAML
 added: v9.3.0
 -->

 * {string[]}

 A list of  the names of all modules provided by Node.js. Can be used to verify
 if a module is maintained by a third-party module or not.

+Note that `Module` in this context isn't the same object that's
+available by default inside file modules. To access it you need to
+require the `Module` module:
+
+```js
+const Module = require('module');
+
+const builtin = Module.builtinModules;
+```
+

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

jasnell

Almost there

jasnell · 2017-12-16T17:54:42Z

doc/api/modules.md

@@ -841,6 +841,14 @@ added: v9.3.0
 A list of  the names of all modules provided by Node.js. Can be used to verify
 if a module is maintained by a third-party module or not.

+Note that `module` in this context isn't the same object that's
+available by default inside file modules. To access it you need to


Please avoid using 'you' in the docs

cjihrig · 2017-12-16T17:55:40Z

doc/api/modules.md

@@ -841,6 +841,14 @@ added: v9.3.0
 A list of  the names of all modules provided by Node.js. Can be used to verify
 if a module is maintained by a third-party module or not.

+Note that `module` in this context isn't the same object that's
+available by default inside file modules. To access it you need to


Instead of saying "available by default inside file modules" maybe say "provided by the module wrapper" and you can link to the module wrapper section.

BridgeAR · 2017-12-16T17:57:25Z

doc/api/modules.md

+
+```js
+const builtin = require('module').builtinModules;
+```


That part is somewhat explained by the brief Accessed via 'require("module")' above.

True, but to be honest I didn't see this my self. I needed to ask an adult for help. So I think it's worth showing an extra time. But maybe it's just me 😅

addressed

maclover7 · 2017-12-16T22:59:58Z

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

Trott · 2017-12-17T04:18:52Z

doc/api/modules.md

@@ -841,11 +841,19 @@ added: v9.3.0
 A list of  the names of all modules provided by Node.js. Can be used to verify
 if a module is maintained by a third-party module or not.

+Note that `module` in this context isn't the same object that's provided


Should this note be moved to below the example for accessing builtinModules? Its placement here makes it in To access it (the next sentence) seem like it refers to the module wrapper which is kind of the opposite of the sentences intention, I think?

I was actually referring to the module mentioned in the headline of this section just above. But I can see that this can be a bit confusing. Not sure if moving it below the code example helps much unless I also change the code example to assign the required module to a constant like I did in the alternative code example in the PR description. What do you think?

This is why I like to refer to this module with an uppercase M as it makes it much easier to distinguish between the two

Trott · 2017-12-17T04:20:00Z

doc/api/modules.md

@@ -841,11 +841,19 @@ added: v9.3.0
 A list of  the names of all modules provided by Node.js. Can be used to verify
 if a module is maintained by a third-party module or not.

+Note that `module` in this context isn't the same object that's provided
+by the [module wrapper][]. To access it require the `Module` module:


~~I don't believe we typically capitalize module names so I suspect this should be the `module` module?~~

Regardless, please insert a comma in the last sentence between it and require: To access it, require the...

Whoops, ignore my comment on casing (module vs. Module) since, you know, that's the whole point... 🙃 Sorry.

Comma added 👍

Trott · 2017-12-17T04:23:57Z

I left some comments/nits, but nothing that I would consider a blocking objection.

maclover7 · 2017-12-22T02:12:57Z

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

maclover7 · 2017-12-22T14:04:57Z

Landing...

maclover7 · 2017-12-22T14:12:54Z

Thank you for your contribution, landed in f4ab204.
❤️ 💚 💙 💛 💜

PR-URL: nodejs#17712 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #17712 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. labels Dec 16, 2017

jasnell previously requested changes Dec 16, 2017

View reviewed changes

cjihrig approved these changes Dec 16, 2017

View reviewed changes

BridgeAR reviewed Dec 16, 2017

View reviewed changes

watson force-pushed the buildins-2 branch from 39e13b8 to 4d1b2bc Compare December 16, 2017 17:59

addaleax approved these changes Dec 16, 2017

View reviewed changes

lpinca approved these changes Dec 16, 2017

View reviewed changes

maclover7 approved these changes Dec 16, 2017

View reviewed changes

Trott reviewed Dec 17, 2017

View reviewed changes

doc: improve module.builtinModules documentation

a1c53c6

watson force-pushed the buildins-2 branch from 4d1b2bc to a1c53c6 Compare December 17, 2017 09:25

jasnell approved these changes Dec 17, 2017

View reviewed changes

watson mentioned this pull request Dec 18, 2017

Support module.builtinModules juliangruber/builtins#13

Closed

maclover7 approved these changes Dec 22, 2017

View reviewed changes

maclover7 self-assigned this Dec 22, 2017

maclover7 closed this Dec 22, 2017

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

gibfahn added the dont-land-on-v6.x label Jan 24, 2018

gibfahn added the dont-land-on-v8.x label Jan 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: improve module.builtinModules documentation #17712

doc: improve module.builtinModules documentation #17712

watson commented Dec 16, 2017

jasnell left a comment

jasnell Dec 16, 2017

watson Dec 16, 2017

cjihrig Dec 16, 2017

watson Dec 16, 2017

BridgeAR Dec 16, 2017

watson Dec 16, 2017

maclover7 commented Dec 16, 2017

Trott Dec 17, 2017 •

edited

Loading

watson Dec 17, 2017

watson Dec 17, 2017

Trott Dec 17, 2017 •

edited

Loading

Trott Dec 17, 2017 •

edited

Loading

Trott Dec 17, 2017

watson Dec 17, 2017

Trott commented Dec 17, 2017 •

edited

Loading

maclover7 commented Dec 22, 2017

maclover7 commented Dec 22, 2017

maclover7 commented Dec 22, 2017

doc: improve module.builtinModules documentation #17712

doc: improve module.builtinModules documentation #17712

Conversation

watson commented Dec 16, 2017

Checklist

Affected core subsystem(s)

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maclover7 commented Dec 16, 2017

Trott Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

Trott Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Dec 17, 2017 • edited Loading

maclover7 commented Dec 22, 2017

maclover7 commented Dec 22, 2017

maclover7 commented Dec 22, 2017

Trott Dec 17, 2017 •

edited

Loading

Trott Dec 17, 2017 •

edited

Loading

Trott Dec 17, 2017 •

edited

Loading

Trott commented Dec 17, 2017 •

edited

Loading