FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: 'constructor' implies use of new keyword by CameronMoorehead · Pull Request #17364 · nodejs/node · GitHub

/ node Public

doc: 'constructor' implies use of new keyword - #17364

Closed
CameronMoorehead wants to merge 3 commits into
nodejs:masterfrom
CameronMoorehead:patch-1
ConversationCommits (3)ChecksFiles changed
Closed

doc: 'constructor' implies use of new keyword#17364
CameronMoorehead wants to merge 3 commits into
nodejs:masterfrom
CameronMoorehead:patch-1

Conversation

CameronMoorehead commented Nov 28, 2017 •
edited by apapirovski
Loading

Copy link
Copy Markdown
Contributor

the square module is described as exporting a constructor,
which would mean it would need to be invoked with the
new keyword in bar.js after requiring it. Otherwise it's
technically a factory function, not a constructor.

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
Affected core subsystem(s)

doc

doc: 'constructor' implies use of new keyword
96c9f1d 
the square module is described as exporting a constructor,
which would mean it would need to be invoked with the
new keyword in bar.js after requiring it. Otherwise it's
technically a factory function, not a constructor.
nodejs-github-bot added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. labels Nov 28, 2017
addaleax approved these changes Nov 28, 2017
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you also turn square into Square in this case? It’s a bit more common to start constructor names with a capital letter

maclover7 approved these changes Nov 28, 2017
View reviewed changes

maclover7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM once @addaleax's comment is addressed

CameronMoorehead commented Nov 28, 2017

Copy link
Copy Markdown
Contributor Author

Very good point. Updated.

Trott commented Nov 28, 2017

Copy link
Copy Markdown
Member

No objection, but a simpler fix might be to leave the code as is and simply remove the words which exports a constructor from the text.

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 8, 2017

tniessen commented Dec 9, 2017

Copy link
Copy Markdown
Member

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

apapirovski pushed a commit that referenced this pull request Dec 9, 2017
doc: 'constructor' implies use of new keyword
1449c18 
The square module is described as exporting a constructor,
which would mean it would need to be invoked with the
new keyword in bar.js after requiring it. Otherwise it's
technically a factory function, not a constructor.

PR-URL: #17364
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

apapirovski commented Dec 9, 2017

Copy link
Copy Markdown
Contributor

Landed in 1449c18

Thanks for the contribution @CameronMoorehead! Congrats on becoming a Contributor!! 🎉

apapirovski closed this Dec 9, 2017

charmander commented Dec 9, 2017

Copy link
Copy Markdown
Contributor

Shouldn’t the example square.js be changed as well? It exports an arrow function, so new will throw outright.

apapirovski commented Dec 9, 2017

Copy link
Copy Markdown
Contributor

@charmander Yes. 👍PR coming up... this is what happens when no one clicks to see the rest of the code 😆

apapirovski mentioned this pull request Dec 9, 2017
Closed
3 tasks
apapirovski added a commit that referenced this pull request Dec 12, 2017
doc: fix modules.md export example
ec6c063 
Arrow functions cannot be called with the new keyword,
convert to ES6 classes instead.

PR-URL: #17579
Refs: #17364
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
doc: 'constructor' implies use of new keyword
6116c3a 
The square module is described as exporting a constructor,
which would mean it would need to be invoked with the
new keyword in bar.js after requiring it. Otherwise it's
technically a factory function, not a constructor.

PR-URL: #17364
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
doc: fix modules.md export example
bc13386 
Arrow functions cannot be called with the new keyword,
convert to ES6 classes instead.

PR-URL: #17579
Refs: #17364
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
doc: 'constructor' implies use of new keyword
1eff647 
The square module is described as exporting a constructor,
which would mean it would need to be invoked with the
new keyword in bar.js after requiring it. Otherwise it's
technically a factory function, not a constructor.

PR-URL: #17364
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
doc: fix modules.md export example
ccffbd9 
Arrow functions cannot be called with the new keyword,
convert to ES6 classes instead.

PR-URL: #17579
Refs: #17364
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins mentioned this pull request Dec 12, 2017
Merged
addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 13, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
doc: 'constructor' implies use of new keyword
f53b4df 
The square module is described as exporting a constructor,
which would mean it would need to be invoked with the
new keyword in bar.js after requiring it. Otherwise it's
technically a factory function, not a constructor.

PR-URL: #17364
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
doc: 'constructor' implies use of new keyword
889986a 
The square module is described as exporting a constructor,
which would mean it would need to be invoked with the
new keyword in bar.js after requiring it. Otherwise it's
technically a factory function, not a constructor.

PR-URL: #17364
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
doc: fix modules.md export example
329f3de 
Arrow functions cannot be called with the new keyword,
convert to ES6 classes instead.

PR-URL: #17579
Refs: #17364
Reviewed-By: Rich Trott <rtrott@gmail.com>
gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
doc: 'constructor' implies use of new keyword
545c526 
The square module is described as exporting a constructor,
which would mean it would need to be invoked with the
new keyword in bar.js after requiring it. Otherwise it's
technically a factory function, not a constructor.

PR-URL: #17364
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
doc: fix modules.md export example
545f5a8 
Arrow functions cannot be called with the new keyword,
convert to ES6 classes instead.

PR-URL: #17579
Refs: #17364
Reviewed-By: Rich Trott <rtrott@gmail.com>
gibfahn mentioned this pull request Dec 20, 2017
Merged
MylesBorins mentioned this pull request Dec 20, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
doc: fix modules.md export example
d85a635 
Arrow functions cannot be called with the new keyword,
convert to ES6 classes instead.

PR-URL: #17579
Refs: #17364
Reviewed-By: Rich Trott <rtrott@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
+2 more reviewers
TimothyGu TimothyGu approved these changes
maclover7 maclover7 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL