★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/17705Nouvelle recherche | Portail wanayoo
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

Jump to bottom

doc: add builtin module in building.md #17705

Closed
wants to merge 1 commit into from
Closed

doc: add builtin module in building.md #17705

wants to merge 1 commit into from

Conversation

Suixinlei
Copy link

@Suixinlei Suixinlei commented Dec 16, 2017 •
edited

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

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Dec 16, 2017

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

@Suixinlei
Copy link
Author

@Suixinlei Suixinlei commented Dec 16, 2017

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

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Dec 16, 2017

@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 Dec 16, 2017
@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
Author

@Suixinlei Suixinlei commented Dec 16, 2017

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

vsemozhetbyt
vsemozhetbyt reviewed Dec 16, 2017
View changes
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

@vsemozhetbyt vsemozhetbyt Dec 16, 2017

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
Author

@Suixinlei Suixinlei Dec 16, 2017

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

@vsemozhetbyt vsemozhetbyt commented Dec 16, 2017

CI lite: https://ci.nodejs.org/job/node-test-pull-request-lite/22/

XadillaX
XadillaX reviewed Dec 18, 2017
View changes
Copy link
Member

@XadillaX XadillaX left a comment

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

joyeecheung
joyeecheung reviewed Dec 18, 2017
View changes
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

@joyeecheung joyeecheung Dec 18, 2017

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

@joyeecheung joyeecheung Dec 18, 2017

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

@joyeecheung joyeecheung Dec 18, 2017

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

@joyeecheung joyeecheung Dec 18, 2017

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

@joyeecheung joyeecheung commented Dec 18, 2017

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

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jan 5, 2018

Ping @Suixinlei

@Suixinlei
Copy link
Author

@Suixinlei Suixinlei commented Jan 9, 2018

sorry, I wll fix this asap.

@Suixinlei
Copy link
Author

@Suixinlei Suixinlei commented Jan 9, 2018

@joyeecheung thx for your advice, fix done.

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 17, 2018

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/96/

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 18, 2018

Landed in f2e62b5, thanks!

joyeecheung added a commit that referenced this issue Jan 18, 2018
@Suixinlei @joyeecheung
doc: add builtin module in building.md
f2e62b5 
Fixes: #12516
Refs: #2497

PR-URL: #17705
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung joyeecheung mentioned this pull request Jan 18, 2018
Closed
evanlucas added a commit that referenced this issue Jan 30, 2018
@Suixinlei @evanlucas
doc: add builtin module in building.md
da2b335 
Fixes: #12516
Refs: #2497

PR-URL: #17705
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas evanlucas mentioned this pull request Jan 30, 2018
Merged
evanlucas added a commit that referenced this issue Jan 30, 2018
@Suixinlei @evanlucas
doc: add builtin module in building.md
926467a 
Fixes: #12516
Refs: #2497

PR-URL: #17705
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this issue Feb 27, 2018
@Suixinlei @MylesBorins
doc: add builtin module in building.md
896aa77 
Fixes: #12516
Refs: #2497

PR-URL: #17705
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this issue Feb 27, 2018
@Suixinlei @MylesBorins
doc: add builtin module in building.md
b83b104 
Fixes: #12516
Refs: #2497

PR-URL: #17705
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 27, 2018
Closed
@MylesBorins MylesBorins mentioned this pull request Feb 27, 2018
Merged
MayaLekova added a commit to MayaLekova/node that referenced this issue May 8, 2018
@Suixinlei @MayaLekova
doc: add builtin module in building.md
092f788 
Fixes: nodejs#12516
Refs: nodejs#2497

PR-URL: nodejs#17705
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XadillaX XadillaX

@jasnell jasnell

@joyeecheung joyeecheung

@vsemozhetbyt vsemozhetbyt

Assignees
No one assigned
Labels
doc module
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

What are Node.js "Linked" modules?
10 participants
@Suixinlei @Fishrock123 @joyeecheung @vsemozhetbyt @BridgeAR @jasnell @XadillaX @MylesBorins @addaleax @nodejs-github-bot