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.

Sign up for GitHub

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

Jump to bottom

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

Closed

CameronMoorehead wants to merge 3 commits into nodejs:master from CameronMoorehead:patch-1

Contributor

CameronMoorehead commented Nov 28, 2017 •

edited by apapirovski

Loading

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 module labels

addaleax approved these changes

View reviewed changes

Member

addaleax left a comment

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

View reviewed changes

Contributor

maclover7 left a comment

LGTM once @addaleax's comment is addressed

CameronMoorehead added 2 commits

November 27, 2017 19:53


          doc: square constructor name capitalized to Square

acc507f


          doc: my Square to mySquare

fb9af62

Contributor Author

CameronMoorehead commented Nov 28, 2017

Very good point. Updated.

cjihrig approved these changes

View reviewed changes

TimothyGu approved these changes

View reviewed changes

Member

Trott commented Nov 28, 2017

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.

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare

December 8, 2017 16:37

addaleax added the author ready label

Member

tniessen commented Dec 9, 2017

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

apapirovski pushed a commit that referenced this pull request


          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>

Member

apapirovski commented Dec 9, 2017

Landed in 1449c18

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

apapirovski closed this

Contributor

charmander commented Dec 9, 2017

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

Member

apapirovski commented Dec 9, 2017

@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

doc: fix modules.md export example #17579

Closed

3 tasks

apapirovski added a commit that referenced this pull request


          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


          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


          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


          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


          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

v9.3.0 proposal #17631

Merged

addaleax removed the author ready label

gibfahn pushed a commit that referenced this pull request


          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


          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 added land-on-v6.x labels

gibfahn pushed a commit that referenced this pull request


          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

v8.9.4 proposal #17772

Closed

gibfahn pushed a commit that referenced this pull request


          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


          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

v8.9.4 proposal #17774

Merged

MylesBorins mentioned this pull request

v6.12.3 proposal #17776

Merged

gibfahn pushed a commit that referenced this pull request


          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>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels