Skip to content
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

Merged
merged 1 commit into from
Nov 29, 2016

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Nov 15, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

doc: improve description of module exports

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

Replace #9552

Fix #9552 (comment)

@sam-github sam-github added the doc Issues and PRs related to the documentations. label Nov 15, 2016
@nodejs-github-bot nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Nov 15, 2016
@iamchenxin
Copy link
Contributor

What about this, i am not sure if i missed something. i can not find the definition of this in old doc.

@sam-github
Copy link
Contributor Author

Above PRs/issues address specific comments you have made.

Do you have more, or can we close this PR?

@iamchenxin
Copy link
Contributor

Yes, seems this will be solved in 9623

@iamchenxin
Copy link
Contributor

And seems document this will make a break change. Cause of some third part tools set different values for this. I should check the babel-plugin again, to test if it is still break this.

@iamchenxin
Copy link
Contributor

Thanks for your patience again, open an issue in babel for asking this.
I gotta sleep, have a good day.

to `module.exports`. As with any variable, if you assign a new value to it, it
is no longer bound to the previous value.
The `exports` variable that is available within a module starts as if
`module.exports` had been assigned to it before your module was evaluated.
Copy link
Member

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 :-)


It allows a shortcut, so that `module.exports.f = ...` can be written more
succinctly as `exports.f = ...`. However, be aware that like any variable, if
you assign a new value to it, it is no longer bound to `module.exports`:
Copy link
Member

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

@sam-github sam-github force-pushed the improve-exports-alias-docs branch 3 times, most recently from d2e5243 to a5ee1d1 Compare November 18, 2016 17:10
@sam-github
Copy link
Contributor Author

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.

succinctly as `exports.f = ...`. However, be aware that like any variable, if
a new value is assigned to `exports`, it is no longer bound to `module.exports`:

module.exports.hello = true; // Exported from require of module
Copy link
Member

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

Copy link
Contributor Author

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.

@sam-github sam-github force-pushed the improve-exports-alias-docs branch 2 times, most recently from 0233696 to 472b2df Compare November 18, 2016 17:59
@Qard
Copy link
Member

Qard commented Nov 19, 2016

LGTM

```

When the `module.exports` property is being completely replaced by a new
object, it is common to also reassign `exports`, for example:
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sam-github
Copy link
Contributor Author

@jasnell can you confirm I made your requested changes?

@sam-github
Copy link
Contributor Author

@jasnell I don't want to merge while your review shows "requested changes" - I made the changes, can you confirm/LGTM?

Copy link
Member

@jasnell jasnell left a 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!

@sam-github
Copy link
Contributor Author

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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@sam-github sam-github merged commit 5887c2b into nodejs:master Nov 29, 2016
addaleax pushed a commit that referenced this pull request Dec 5, 2016
- 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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
2 tasks
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
- 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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
- 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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
- 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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
- 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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This was referenced Dec 21, 2016
@sam-github sam-github deleted the improve-exports-alias-docs branch April 17, 2017 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants