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 builtin module in building.md #17705

Closed
wants to merge 1 commit into from

Conversation

Suixinlei
Copy link
Contributor

@Suixinlei Suixinlei commented Dec 16, 2017

Add builtin module doc in BUILDING.md and introduce --link-modules in doc.

Fixes: #12516
Refs: #2497

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

doc

@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 Dec 16, 2017
@Fishrock123
Copy link
Contributor

I feel like this documentation belongs in the building docs and not in the API docs.

@Suixinlei
Copy link
Contributor Author

Thank you for your advice.Should I close this pr and open the other one? @Fishrock123

@joyeecheung
Copy link
Member

@Suixinlei You can just move the changes to BUILDING.md, amend the commit and force push to the PR branch, the PR description still applies if you move the changes there so I don't think it's necessary to close this one and open a new PR.

@Suixinlei Suixinlei force-pushed the doc-builtin-module branch 2 times, most recently from 2d48d8f to 478d696 Compare December 16, 2017 07:58
@Suixinlei Suixinlei changed the title doc: add builtin module in module.md doc: add builtin module in building.md Dec 16, 2017
@Suixinlei
Copy link
Contributor Author

@joyeecheung done. I've updated the pr and the commit.

BUILDING.md Outdated

## Building Node.js with external core modules

In general that core modules are only located in `lib/` folder. Node can
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!
Some tiny fixes:

  1. that core modules -> those core modules.
  2. Node -> Node.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vsemozhetbyt Thanks a lot. I've already update my code.

@Suixinlei Suixinlei force-pushed the doc-builtin-module branch 2 times, most recently from ad10522 to 615f11a Compare December 16, 2017 13:32
@vsemozhetbyt
Copy link
Contributor

Copy link
Contributor

@XadillaX XadillaX left a comment

Choose a reason for hiding this comment

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

I think the commit message shouldn't include Fixes since it's just a documentation changing. Maybe we can use Refs instead.

BUILDING.md Outdated

## Building Node.js with external core modules

In general those core modules are only located in `lib/` folder. Node.js can
Copy link
Member

Choose a reason for hiding this comment

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

The word "those" here looks a bit unnatural since the core modules are not mentioned before this section, and the fact that they are in the lib/ folder is not entirely relevant here since the internal modules are not exposed to users. I would suggest something simpler like:

It is possible to specify one or more JavaScript text files to be bundled in the binary as builtin modules
when building Node.js.

BUILDING.md Outdated
specify one or more JavaScript text files to be bundled in the binary as
builtin modules.

##### Unix / macOS
Copy link
Member

Choose a reason for hiding this comment

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

We don't really need these many levels here, I think ### is enough.

BUILDING.md Outdated
##### Unix / macOS

```console
$ ./configure --link-module '/root/myModule.js'
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an explanation for this command? Maybe something like To make `/root/myModule.js` available via `require('root/myModule')`:

BUILDING.md Outdated
##### Windows

```console
> .\vcbuild link-module './myModule.js'
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an explanation for this command as well?

@joyeecheung
Copy link
Member

BTW this does not fix #12516 which was asking about the bundled native modules (addons), not the JS modules.

@BridgeAR
Copy link
Member

BridgeAR commented Jan 5, 2018

Ping @Suixinlei

@Suixinlei
Copy link
Contributor Author

sorry, I wll fix this asap.

@Suixinlei
Copy link
Contributor Author

@joyeecheung thx for your advice, fix done.

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 17, 2018
@joyeecheung
Copy link
Member

Landed in f2e62b5, thanks!

joyeecheung pushed a commit that referenced this pull request Jan 18, 2018
Fixes: #12516
Refs: #2497

PR-URL: #17705
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2018
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
Fixes: #12516
Refs: #2497

PR-URL: #17705
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
Fixes: #12516
Refs: #2497

PR-URL: #17705
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
Fixes: #12516
Refs: #2497

PR-URL: #17705
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
Fixes: #12516
Refs: #2497

PR-URL: #17705
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This was referenced Feb 27, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Fixes: nodejs#12516
Refs: nodejs#2497

PR-URL: nodejs#17705
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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.

What are Node.js "Linked" modules?
10 participants