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: clarify relation between a file and a module #9026

Closed
wants to merge 2 commits into from
Closed

doc: clarify relation between a file and a module #9026

wants to merge 2 commits into from

Conversation

marzelin
Copy link
Contributor

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

doc

Description of change

I've found this sentence: In Node.js, files and modules are in one-to-one correspondence. a bit vague, so I propose replacing it with a more informative one.

I've found this sentence: `In Node.js, files and modules are in one-to-one correspondence.` a bit vague, so I propose replacing it with a more informative one.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. labels Oct 11, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Oct 11, 2016

Please wrap lines at 80 characters.

@@ -4,8 +4,7 @@

<!--name=module-->

Node.js has a simple module loading system. In Node.js, files and modules are
in one-to-one correspondence. As an example, `foo.js` loads the module
Node.js has a simple module loading system. In Node.js each JavaScript file is a separate module which means that all variables, constants, functions, etc. declared in the file can be accessed only in that specific file unless explicitly exposed using `exports`-`require` interface. As an example, `foo.js` loads the module
Copy link
Contributor

Choose a reason for hiding this comment

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

Node supports more than just JavaScript source files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean json, c++ addons? Are they treated as modules, too?

Copy link
Member

Choose a reason for hiding this comment

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

You mean json, c++ addons? Are they treated as modules, too?

Yes. :)

@@ -4,8 +4,7 @@

<!--name=module-->

Node.js has a simple module loading system. In Node.js, files and modules are
in one-to-one correspondence. As an example, `foo.js` loads the module
Node.js has a simple module loading system. In Node.js each JavaScript file is a separate module which means that all variables, constants, functions, etc. declared in the file can be accessed only in that specific file unless explicitly exposed using `exports`-`require` interface. As an example, `foo.js` loads the module
Copy link
Contributor

Choose a reason for hiding this comment

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

Things can also be exposed outside of a module if they are declared as globals, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean adding something to global object? global is there but it should be the last resort or never used at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to accidentally creating global variables, but yes, you could do that too. Definitely not something that we want to recommend, but still technically possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

omg Node doesn't use strict mode by default. That changes things a little bit. Current module system doesn't protect from polluting global namespace, but just reads and executes specified files. I hope Node.js will soon support ES2015 modules. Thanks for pointing this out.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 11, 2016

TBH, I find the existing text to be more clear than the proposed changes.

@marzelin
Copy link
Contributor Author

@cjihrig You know all Node ins and outs, so your point of view is different from somebody who just start out with this runtime. Is it too simplistic for you? (of course after sorting out mistakes you pointed out).

Added short clarification
@marzelin
Copy link
Contributor Author

I've changed the text as suggested in the comments. What do you think about it now?

@gibfahn
Copy link
Member

gibfahn commented Oct 12, 2016

Updated text LGTM (I think you've answered @cjihrig's comments)

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
jasnell pushed a commit that referenced this pull request Oct 26, 2016
PR-URL: #9026
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 26, 2016

Landed in f45eb16

@jasnell jasnell closed this Oct 26, 2016
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
PR-URL: #9026
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2016
PR-URL: #9026
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2016
PR-URL: #9026
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 19, 2016
PR-URL: #9026
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This was referenced Nov 22, 2016
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.

8 participants