FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build: rectify --link-module usage by pmq20 · Pull Request #3379 · nodejs/node · GitHub

/ node Public

build: rectify --link-module usage - #3379

Closed
pmq20 wants to merge 1 commit into
nodejs:masterfrom
pmq20:master-psvr-link-module
Closed

build: rectify --link-module usage#3379
pmq20 wants to merge 1 commit into
nodejs:masterfrom
pmq20:master-psvr-link-module

Conversation

pmq20 commented Oct 15, 2015

Copy link
Copy Markdown
Contributor

Modules imported by --link-module are not referenced by name but actually by path.

silverwind added the build Issues and PRs related to build files or the CI. label Oct 15, 2015

mscdex commented Oct 15, 2015

Copy link
Copy Markdown
Contributor

I'm not sure this was the intended behavior.

@bmeck Thoughts?

Copy link
Copy Markdown
Contributor

ping @bmeck

bmeck commented Oct 20, 2015

Copy link
Copy Markdown
Member

The original intent was to have as little magic as possible since the mapping of some things can be complex and not a single name (such as node's internal use of 'internal/*'). I would be more prone to allow mapping a path to a native name rather than always do this. Basically, I want it to have as little magic as possible personally.

That said, there is a use case for mapping paths to different builtin names. Perhaps discussing that prior to merging anything would be for the best.

Copy link
Copy Markdown
Contributor

@bmeck this is just clarifying the configure option to what it actual does in more detail. LGTM if it LGTY.

bmeck commented Oct 20, 2015

Copy link
Copy Markdown
Member

@Fishrock123 sounds good. LGTM

Fishrock123 pushed a commit that referenced this pull request Oct 20, 2015
PR-URL: #3379
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Copy link
Copy Markdown
Contributor

Thanks, landed in 753509f

rvagg pushed a commit that referenced this pull request Oct 21, 2015
PR-URL: #3379
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
rvagg mentioned this pull request Oct 21, 2015

Copy link
Copy Markdown
Contributor

should be considered for LTS

/cc @jasnell

jasnell commented Oct 24, 2015

Copy link
Copy Markdown
Member

Can but not sure there's a critical need for it

jasnell pushed a commit that referenced this pull request Oct 30, 2015
PR-URL: #3379
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

jasnell commented Oct 30, 2015

Copy link
Copy Markdown
Member

Landed in v4.x-staging in ad65c82

jasnell pushed a commit that referenced this pull request Oct 30, 2015
PR-URL: #3379
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Issues and PRs related to build files or the CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL