GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: clarify relation between a file and a module by marzelin · Pull Request #9026 · nodejs/node

Conversation

@marzelin

@marzelin marzelin commented Oct 11, 2016

Copy link
Copy Markdown
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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@marzelin
doc: clarify relation between a file and a module
9472278 
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 Node.js documentation. module Issues and PRs related to the module subsystem. labels Oct 11, 2016
@cjihrig

cjihrig commented Oct 11, 2016

Copy link
Copy Markdown
Contributor

Please wrap lines at 80 characters.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig reviewed Oct 11, 2016
View reviewed changes
Comment thread doc/api/modules.md Outdated

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

@cjihrig cjihrig Oct 11, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@marzelin marzelin Oct 11, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@addaleax addaleax Oct 11, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
cjihrig
cjihrig reviewed Oct 11, 2016
View reviewed changes
Comment thread doc/api/modules.md Outdated

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

@cjihrig cjihrig Oct 11, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@marzelin marzelin Oct 11, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjihrig cjihrig Oct 11, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@marzelin marzelin Oct 11, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@cjihrig

cjihrig commented Oct 11, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@marzelin

marzelin commented Oct 11, 2016

Copy link
Copy Markdown
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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@marzelin
doc: remove controversial parts
302de18 
Added short clarification
@marzelin

marzelin commented Oct 12, 2016

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gibfahn
gibfahn approved these changes Oct 12, 2016
View reviewed changes
@gibfahn

gibfahn commented Oct 12, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca approved these changes Oct 12, 2016
View reviewed changes

@lpinca lpinca left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@rvagg
rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
jasnell
jasnell approved these changes Oct 19, 2016
View reviewed changes
jasnell pushed a commit that referenced this pull request Oct 26, 2016
@marzelin @jasnell
doc: clarify relation between a file and a module
f45eb16 
PR-URL: #9026
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell

jasnell commented Oct 26, 2016

Copy link
Copy Markdown
Member

Landed in f45eb16
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Oct 26, 2016
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
@marzelin @evanlucas
doc: clarify relation between a file and a module
3f90481 
PR-URL: #9026
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@evanlucas evanlucas mentioned this pull request Nov 3, 2016
v7.1.0 proposal - 2016-11-08 #9438
Merged
MylesBorins pushed a commit that referenced this pull request Nov 17, 2016
@marzelin Myles Borins
doc: clarify relation between a file and a module
1ad77ad 
PR-URL: #9026
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins added land-on-v4.x labels Nov 17, 2016
MylesBorins pushed a commit that referenced this pull request Nov 17, 2016
@marzelin Myles Borins
doc: clarify relation between a file and a module
0f41058 
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
@marzelin @MylesBorins
doc: clarify relation between a file and a module
a5860b4 
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
v6.9.2 proposal #9735
Merged
v4.7.0 proposal #9736
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax left review comments
@cjihrig cjihrig cjihrig left review comments
@jasnell jasnell jasnell approved these changes
@lpinca lpinca lpinca approved these changes
+1 more reviewer
@gibfahn gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

doc Issues and PRs related to Node.js documentation. 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.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@marzelin @cjihrig @gibfahn @jasnell @addaleax @lpinca @MylesBorins @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.