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

docs: add docs to require() #23605

Closed
wants to merge 2 commits into from

Conversation

@ErickWendel
Copy link
Contributor

commented Oct 12, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

#23106

@ErickWendel ErickWendel force-pushed the ErickWendel:docs/require branch from a3c6917 to 7895992 Oct 12, 2018

doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/modules.md Outdated Show resolved Hide resolved
@ChALkeR

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

@vsemozhetbyt

This comment has been minimized.

@@ -552,7 +552,18 @@ added: v0.1.13

* {Function}

To require modules.
Used to import modules, `JSON` and local files. Modules can be imported from `node_modules`. Local modules and JSON files can be imported using the pattern `'./`.

This comment has been minimized.

Copy link
@lundibundi

lundibundi Oct 12, 2018

Member

The general rule is to have lines be at most 80 chars, though this can be fixed at landing.

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

This probably should be squashed (could be done at landing).

@ErickWendel ErickWendel force-pushed the ErickWendel:docs/require branch from a88cac9 to e908b9e Oct 12, 2018

@ErickWendel

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

This probably should be squashed (could be done at landing).

squashed

@@ -552,7 +552,18 @@ added: v0.1.13

* {Function}

To require modules.
Used to import modules, `JSON` and local files. Modules can be imported from `node_modules`. Local modules and JSON files can be imported using the pattern `'./`.

This comment has been minimized.

Copy link
@gireeshpunathil

gireeshpunathil Oct 13, 2018

Member

is there a single quote missing between the back-quotes?

This comment has been minimized.

Copy link
@ErickWendel

ErickWendel Oct 13, 2018

Author Contributor

fixed! :D

@ErickWendel ErickWendel force-pushed the ErickWendel:docs/require branch from e908b9e to 18f6f8e Oct 13, 2018

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 15, 2018
doc: add review suggestions to require()
PR-URL: nodejs#23605
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@a0viedo
Copy link
Member

left a comment

LGTM

@@ -552,7 +552,18 @@ added: v0.1.13

* {Function}

To require modules.
Used to import modules, `JSON` and local files. Modules can be imported from `node_modules`. Local modules and JSON files can be imported using the pattern `'./'`.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Oct 15, 2018

Contributor

Nit: This line is more than 80 characters I believe.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Oct 15, 2018

Contributor

Nit: Also, mentioning "relative path" would be better here, maybe with ./ in the brackets.

Used to import modules, `JSON` and local files. Modules can be imported from `node_modules`. Local modules and JSON files can be imported using the pattern `'./'`.

```js
// importing localModule

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Oct 15, 2018

Contributor

Nit: Importing a local module.

// importing JSON file
const jsonData = require('./path/filename.json');
// importing module from node_modules or Node.js internals

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Oct 15, 2018

Contributor

Nit: Node.js built-in module would be better I believe

[Squash] nits and clarification
Addressing nits and clarifications
@jasnell

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@ErickWendel .. certainly hope you don't mind, but I went ahead and pushed an additional commit that handled @thefourtheye's nits and added a further clarification.

Used to import modules, `JSON` and local files. Modules can be imported from `node_modules`. Local modules and JSON files can be imported using the pattern `'./'`.
Used to import modules, `JSON`, and local files. Modules can be imported
from `node_modules`. Local modules and JSON files can be imported using
a relative path (e.g. `./`, `./foo`, `./bar/baz`, `../foo`) that will be

This comment has been minimized.

Copy link
@lundibundi

lundibundi Oct 16, 2018

Member

Should we perhaps also note that path-resolution is OS independent (given that we are providing examples in Linux/Unix flavor)?

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Oct 17, 2018

Member

@lundibundi I think that would be a good addition but it could also land in a separate PR?

@apapirovski

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

@Trott

This comment has been minimized.

Copy link
Member

commented Oct 24, 2018

Landed in 51cd971

@Trott Trott closed this Oct 24, 2018

Trott added a commit to Trott/io.js that referenced this pull request Oct 24, 2018
doc: add review suggestions to require()
PR-URL: nodejs#23605
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

commented Oct 24, 2018

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

targos added a commit that referenced this pull request Oct 24, 2018
doc: add review suggestions to require()
PR-URL: #23605
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos referenced this pull request Oct 27, 2018
MylesBorins added a commit that referenced this pull request Nov 26, 2018
doc: add review suggestions to require()
PR-URL: #23605
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Nov 26, 2018
doc: add review suggestions to require()
PR-URL: #23605
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere referenced this pull request Nov 27, 2018
rvagg added a commit that referenced this pull request Nov 28, 2018
doc: add review suggestions to require()
PR-URL: #23605
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg added a commit that referenced this pull request Nov 28, 2018
doc: add review suggestions to require()
PR-URL: #23605
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Nov 29, 2018
doc: add review suggestions to require()
PR-URL: #23605
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere referenced this pull request Nov 29, 2018
@BethGriggs BethGriggs referenced this pull request Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.