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: add more detailed illustration for module.exports and exports. #9552

Closed
wants to merge 2 commits into from

Conversation

iamchenxin
Copy link
Contributor

@iamchenxin iamchenxin commented Nov 11, 2016

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

doc

Description of change
  1. Since there is an illustration for The module wrapper, the module.exports could be clearly explained with the logic of source code.
  2. Add notice for avoid using this to export values in a module file.
  • some babel-plugin, like transform-es2015-modules-commonjs will break this to undefined, since this is undocumented, i thought babel-plugin is free to break, and user should not use it.

1. Since there is an illustration for `The module wrapper`, the `module.exports` could be clearly explained with the logic of source code.
2. Add notice for `this` of a module file. (Im not sure if it is a bug of `transform-es2015-modules-commonjs`, but since `this` is undocumented, i thought `babel-plugin` is free to break it.)
@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 Nov 11, 2016
this, assign the desired export object to `module.exports`. Note that assigning
the desired object to `exports` will simply rebind the local `exports` variable,
which is probably not what you want to do.
`module.exports` object is used to export values in a js file. It is
Copy link
Member

Choose a reason for hiding this comment

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

  • I don't understand why The was removed as the first word of this sentence.
  • Do not use js as an abbreviation for JavaScript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The must be my style error.
I am not sure how to name that? a module? a module file? a Javascript file? or help me to get a right word for this.

the desired object to `exports` will simply rebind the local `exports` variable,
which is probably not what you want to do.
`module.exports` object is used to export values in a js file. It is
initialized as `{}` by Module system, and passed to `The module wrapper` as argument(This makes `module.exports` and `exports` available in a js file,
Copy link
Member

Choose a reason for hiding this comment

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

  • Line wrap at 80 columns
  • Do not put The module wrapper in backticks.
  • Put a space before the opening parenthesis.
  • Move parenthetical to its own sentence and include punctuation.

which is probably not what you want to do.
`module.exports` object is used to export values in a js file. It is
initialized as `{}` by Module system, and passed to `The module wrapper` as argument(This makes `module.exports` and `exports` available in a js file,
the details will be described below). `require` return the processed
Copy link
Member

Choose a reason for hiding this comment

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

  • return -> returns

`module.exports` object is used to export values in a js file. It is
initialized as `{}` by Module system, and passed to `The module wrapper` as argument(This makes `module.exports` and `exports` available in a js file,
the details will be described below). `require` return the processed
`module.exports` as the exports of a module.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure what is meant by "the exports of a module" but I'm pretty sure that phrasing can be clarified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im not sure how to name this, could you help me.

module.exports.emit('ready');
}, 1000);
```
There is no magic behind `exports`, it is the `module.exports` passed to
Copy link
Member

Choose a reason for hiding this comment

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

  • Fix the comma splice.

}, 1000);
```
There is no magic behind `exports`, it is the `module.exports` passed to
`The module wrapper` as the first argument.
Copy link
Member

Choose a reason for hiding this comment

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

No back ticks and no capitalization of The in The module wrapper. Are you trying to make it a link or something? I'm puzzled by the repeated backticks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cause The module wrapper is a term which name previously, i thought i would has a backticks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes The module wrapper to module wrapper

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

I have no comments on the content (yet, at least) and I appreciate that a lot of effort went into writing this, but it needs editing for style, grammar, formatting, etc. Maybe someone on @nodejs/documentation can jump in and give it an edit?

@iamchenxin
Copy link
Contributor Author

iamchenxin commented Nov 13, 2016

@Trott Thanks for your reviewing. Im poor at english, so very sorry for my style, grammar, formatting and etc. Will try to make this better, and it would be nice if there is some one to help me to get a qualified content.

Line wrap at 80 columns.
comma splice.
`The module wrapper` to [module wrapper](#modules_the_module_wrapper)
parenthesis error.
@iamchenxin
Copy link
Contributor Author

iamchenxin commented Nov 13, 2016

Leaving "js file" and "the exports of a module" unmodified, im not sure what are suitable phrases for them, please help me.

@Trott
Copy link
Member

Trott commented Nov 14, 2016

I'll do the editing if no one more qualified jumps in, but I'd love it if someone else (maybe from @nodejs/documentation) volunteered.

Might be a good idea to get a review of the contents first. Anyone?

Also: Opinions as to whether this belongs in the modules doc or if it's the type of material that might be better as a separate guide.

@nodejs/collaborators

which is probably not what you want to do.
The `module.exports` object is used to export values in a js file. It is
initialized as `{}` by Module system, and passed to
[module wrapper](#modules_the_module_wrapper) as argument. (This makes
Copy link
Contributor

Choose a reason for hiding this comment

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

"as an argument". Remove the parentheses around the complete sentence.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: replace:

... a js file.

with:

... `*.js` files.

The `module.exports` object is used to export values in a js file. It is
initialized as `{}` by Module system, and passed to
[module wrapper](#modules_the_module_wrapper) as argument. (This makes
`module.exports` and `exports` available in a js file, the details will be
Copy link
Contributor

Choose a reason for hiding this comment

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

", the details..." unnecessary, remove, or replace with a "see LINK" that links to the docs.

module.exports.emit('ready');
}, 1000);
```
There is no magic behind `exports`. It is the `module.exports` passed to
Copy link
Contributor

Choose a reason for hiding this comment

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

remove "there is no magic" phrase, it adds no value. "It is the" change to "exports is an alias to the". Link module.exports to its docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key point of this sentence is i want to remove the alias, because of this word make some one like me very confused , till check the source code. alias tend to refer to something like reference, imply the things happened in export will reflect into module.export. But it is indeed a value copy from module.export by argument.
I am not sure how to describe this rightly.


As described in [module wrapper](#modules_the_module_wrapper), your js file
Copy link
Contributor

Choose a reason for hiding this comment

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

js -> Javascript

Copy link
Member

Choose a reason for hiding this comment

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

Please avoid the use of informal pronouns such as 'you', 'your', 'us', etc.

});
```
And this is a minimized logic of `require` for `module.exports`:
```js
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of this code snippet?

above, and call it like the code logic described in `_require`. By the code
logic, behaviors of `module.exports` and `exports` are dependent on these
points:
- `require` is a synchronize process.
Copy link
Contributor

Choose a reason for hiding this comment

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

synchronize -> synchronous

logic, behaviors of `module.exports` and `exports` are dependent on these
points:
- `require` is a synchronize process.
- `require` return the `module.exports` as the exports of a module.
Copy link
Contributor

Choose a reason for hiding this comment

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

"returns"

points:
- `require` is a synchronize process.
- `require` return the `module.exports` as the exports of a module.
- Javascript always passes by value.
Copy link
Contributor

Choose a reason for hiding this comment

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

javascript always passes what by value?


As a result, the export results of a module is a returned value of
`module.exports` at the tickcount which `require` is returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the behaviors of this. I thought whenever the user do anything in a file 'x.js', the result of require('./x.js') in 'y.js', will always be the returned value of module.exports.
Because my English is poor, so sorry for my inexactly description.

Copy link
Contributor

Choose a reason for hiding this comment

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

// module "hello.js"
module.exports.yes = true;
setTimeout(function() {
module.exports.yes = false;
module.exports.somethingElse = true;
}, 1000);

I don't know exactly what you are trying to say, but I believe you are saying require(..../hello.js).yes === true always and forevere because it was true in the tick that require was called, but that isn't true. 1 second after the require, the module's API value will change.

Copy link
Contributor Author

@iamchenxin iamchenxin Nov 15, 2016

Choose a reason for hiding this comment

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

nope, i want to say it is the value of the returned module.exports.
Cause in Javascript value of an Object is a memory location(or Handle?).
not sure how to describe the term value rightly. ( and there is a term deep value in Javascript, but in some place the value is used to refer to deep value).

Copy link
Contributor Author

@iamchenxin iamchenxin Nov 15, 2016

Choose a reason for hiding this comment

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

this is why i write a pseudo code for require, a pseudo code will always exactly and cross langauges and terms. (But if i am good at English, may be i do not need that pseudo code to explain what it should be.)

done in any callbacks. This does not work:

x.js:
Let's illustrate the behaviors and principles with codes. (You can use either
Copy link
Contributor

Choose a reason for hiding this comment

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

@Trott is this colloquial "let's" and "you" used in node documentation?

"codes" -> "code"

return module.exports;
}
```
When you `require` a file, Node.js will wrap it with a function wrapper as
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a useful way to describe require. If you want to read the code to understand the behaviour, the code is there, its open source. Docs should describe behaviour and interface, not just reformat the code and expect the reader to read the code to figure out how it behaves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, this is me main purpose of the PR, cause of the old docs of module.export did not describe what the module.export should be and should not be, so this led to many people repeatedly ask what is a module.export and export in stackoverflow. previously if some one want to make sure what is a module.export, the only way is read the source code. Want to make this easier.

I thought _require is a pseudocode more than a code of reformating. Cause an exactly description maybe need hundreds of words(maybe will make the docs like a specification), so i thought a pseudocode is suitable here, and i checked it again, it is the things happened for module.export in currently source code.

The pseudocode make thes behaviors and interface of module.exports and exports obviously.
I think there should be an other alternatively good way to describe the behaviors and interface, but i am not be able to do that, cause of my poor English.

module.exports = { a: 'hello' };
}, 0);
```
function synchronize(exports, _, module) { // the module wrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a useful way to describe how to write modules, since users do not actually write modules like this

`exports` to export values previously, But since `this` is an undocumented
value, the behavior of `this` is not guaranteed. Ex: when you use `babel`,
`transform-es2015-modules-commonjs` will break `this`, so you should not use
`this` as a shortcut for export.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't agree with the above. this is documented, its documented right here! And if the module does not work when some random thirdparty tool is used to convert it to some other form... that should be part of that tools documentation, not proposed as a general rule for node modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did do find this in old module docs. searched in doc/api/module.md again previously.

`this` as a shortcut for export.

**As a guideline**, never use 'this' to export, and if the `exports` and
`module.exports` makes you confused, ignore `exports` and only use
Copy link
Contributor

Choose a reason for hiding this comment

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

if you are still confused at this point, the docs have failed. I don't see any description here about the real difference between the two:

exports is more brief to use, and is good when just exporting functions

module.exports is necessary to use when a module should return something other than the default object provided as an export value, such as a function

If you want to add guidelines, the guideline should be to redefine exports and module.exports at the same time:

module.exports = exports = MyConstructorFunction;

@sam-github
Copy link
Contributor

IMO, the attempt to demonstrate how the module system works by building a mini version of it is best left to a guide (or a blog post).

@iamchenxin
Copy link
Contributor Author

@sam-github Thanks for you reviewing, it is my rough idea of making the module.exports and exports more clearly in docs. Want to make the user no longer need to check the source code again to understand the behaviors of them.

But cause of my poor English( and also i am new to nodejs, there are many things i dont know), i has a lot of weird descriptions for my idea.

After reading the reviewing, i have some thought:
1 should remove many of the explanations from this PR. ( explanations should not be flooded docs. )
2 make a good definition of module.exports and exports.
3 make an exactly description of the behaviors and interface.

Cause of my poor English, not sure if i has ability to achieve 2 and 3.
With many thanks to your reviewing.

@sam-github
Copy link
Contributor

@iamchenxin If comfort writing in English is a problem for you, you could perhaps open an issue, describing what it was that you did not understand from the docs, and someone more comfortable in writing docs could then fix them for you, and you could review.

Its a bit hard for me, I've found the docs clear, but if you didn't, then they need improving... but how specifically?

@sam-github
Copy link
Contributor

The key point of this sentence is i want to remove the alias, because of this word make some one like me very confused , till check the source code. alias tend to refer to something like reference, imply the things happened in export will reflect into module.export. But it is indeed a value copy from module.export by argument. I am not sure how to describe this rightly.

Thinks happening in export will be reflected in module.export!

export.fu = true;
assert.equal(module.export, true);

Honestly, I just re-read the docs, and they seem to describe this perfectly well: https://nodejs.org/api/modules.html#modules_module_exports

Can you point to the stackoverflow questions?

At this point, I'm -1 on this PR. It makes a number of things less clear, and doesn't appear to me to help with the actual problem.

Would changing "alias" to "local variable" be more clear?

@iamchenxin
Copy link
Contributor Author

@sam-github nope, it is not just for me, if you google what is module.export, will see a lot of explanations try to explain what it is. I think the definition of module.export and its behavior should be wrote in the docs.
Maybe you can cost a little time to write a definition and a behavior specification for module.export and export. and also this in top level. ( I think my PR will save some time for another PR which really write the definition.)
And if i have some misunderstanding for the Rules of Nodejs's PR, forgive me. (I will follow the rule next time, cause of i did not know it previously)

@sam-github
Copy link
Contributor

You have only two semi-actionable comments right now:

  1. some people don't understand, please reference. For example, I can google, but I find http://stackoverflow.com/questions/5311334/what-is-the-purpose-of-node-js-module-exports-and-how-do-you-use-it, which refers to ancient 4.x versions of the documentation which have since been improved to address exactly this kind of question.
  2. you found the word "alias" confusing - what do you think of my suggestion to change to "local variable"

Its hard to make improvements without more specific feedback.

What exactly didn't you understand?

@iamchenxin
Copy link
Contributor Author

about alias, it is something about by value and by reference confusing in Javascript.
Cause of in Javascript there is no term memory location or pointer or reference.
So alias is a confusing word here.
Often, if a is an alias of b, if a is set to 'hello'. b should be hello too.

@iamchenxin
Copy link
Contributor Author

@sam-github I have the idea to improve the old doc when i try to mock functions exported from a module, and find there is not enough informations in docs for exports.

exactly there is nothing im confusing now, cause of reading source code. ( The purpose is to make others do not need to ask questions for what is module.export, like 1 stackoverflow. should make people to get the exactly description from official API doc, not from pieces answers from web.)

2 local variable is better than alias.

And what about this. Is this defined?

@sam-github
Copy link
Contributor

#9622

@iamchenxin
Copy link
Contributor Author

@sam-github Thanks for your patience to me. I make a lot of trouble to you. i move to #9622.

@sam-github
Copy link
Contributor

Its my pleasure to help, if I can.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

While I definitely appreciate the effort to improve clarity in the docs around this, this particular edit has a number of deficiencies currently. I left a few comments.

@iamchenxin iamchenxin closed this Nov 18, 2016
@iamchenxin
Copy link
Contributor Author

@jasnell Thanks for reviewing, this PR was replaced by #9622 .

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.

5 participants