-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
doc: improve description of module exports
#9622
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.
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
doc: improve description of module exports
#9622
Conversation
28b6e92 to
84b18c2
Compare
|
What about |
|
Above PRs/issues address specific comments you have made. Do you have more, or can we close this PR? |
|
Yes, seems |
|
And seems document this will make a break change. Cause of some third part tools set different values for |
|
Thanks for your patience again, open an issue in babel for asking |
doc/api/modules.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid the use of informal pronouns such as you, your and us in the docs :-)
doc/api/modules.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, this last sentence is better written as: "Note, however, assigning a new value to exports causes it to no longer be bound to module.exports."
d2e5243 to
a5ee1d1
Compare
|
Thanks @jasnell, I removed the colloquialisms, and also fixed another bug in the require pseudo-code. Personally, I would remove the pseudo code in preference for the textual description and example, but maybe some other time, so it doesn't slow this PR. |
doc/api/modules.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should be wrapped in an explicit code block using the three backticks...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks, sorry for the churn, I'll be up to speed on current conventions soon.
0233696 to
472b2df
Compare
|
LGTM |
doc/api/modules.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really widely used or a good practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't say it's universal, but I see it use fairly often as a defensive measure. I'd consider it a relatively good practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@jasnell can you confirm I made your requested changes? |
472b2df to
e8f3290
Compare
|
@jasnell I don't want to merge while your review shows "requested changes" - I made the changes, can you confirm/LGTM? |
jasnell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm! Sorry for the delay!
|
Thanks! |
- Do not use word alias, it isn't well defined - Fix return value of require() example, which confusingly was not the exported API as it should have been. - Fix the require() example, which claimed that the module exported `0`, when it exports `some_func`. - Describe best practice in keeping exports and module.exports bound together. - Describe why exports exists - Remove reference to magic, which is also not well defined PR-URL: nodejs#9622 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: James M Snell <jasnell@gmail.com>
e8f3290 to
5887c2b
Compare
- Do not use word alias, it isn't well defined - Fix return value of require() example, which confusingly was not the exported API as it should have been. - Fix the require() example, which claimed that the module exported `0`, when it exports `some_func`. - Describe best practice in keeping exports and module.exports bound together. - Describe why exports exists - Remove reference to magic, which is also not well defined PR-URL: #9622 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Do not use word alias, it isn't well defined - Fix return value of require() example, which confusingly was not the exported API as it should have been. - Fix the require() example, which claimed that the module exported `0`, when it exports `some_func`. - Describe best practice in keeping exports and module.exports bound together. - Describe why exports exists - Remove reference to magic, which is also not well defined PR-URL: #9622 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Do not use word alias, it isn't well defined - Fix return value of require() example, which confusingly was not the exported API as it should have been. - Fix the require() example, which claimed that the module exported `0`, when it exports `some_func`. - Describe best practice in keeping exports and module.exports bound together. - Describe why exports exists - Remove reference to magic, which is also not well defined PR-URL: #9622 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Do not use word alias, it isn't well defined - Fix return value of require() example, which confusingly was not the exported API as it should have been. - Fix the require() example, which claimed that the module exported `0`, when it exports `some_func`. - Describe best practice in keeping exports and module.exports bound together. - Describe why exports exists - Remove reference to magic, which is also not well defined PR-URL: #9622 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Do not use word alias, it isn't well defined - Fix return value of require() example, which confusingly was not the exported API as it should have been. - Fix the require() example, which claimed that the module exported `0`, when it exports `some_func`. - Describe best practice in keeping exports and module.exports bound together. - Describe why exports exists - Remove reference to magic, which is also not well defined PR-URL: #9622 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Checklist
Affected core subsystem(s)
doc
Description of change
doc: improve description of module
exportsReplace #9552
Fix #9552 (comment)