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

build: rectify --link-module usage #3379

Closed
wants to merge 1 commit into from

Conversation

pmq20
Copy link
Contributor

@pmq20 pmq20 commented Oct 15, 2015

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

@silverwind silverwind added the build Issues and PRs related to build files or the CI. label Oct 15, 2015
@mscdex
Copy link
Contributor

mscdex commented Oct 15, 2015

I'm not sure this was the intended behavior.

@bmeck Thoughts?

@Fishrock123
Copy link
Contributor

ping @bmeck

@bmeck
Copy link
Member

bmeck commented Oct 20, 2015

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.

@Fishrock123
Copy link
Contributor

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

@bmeck
Copy link
Member

bmeck commented Oct 20, 2015

@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>
@Fishrock123
Copy link
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 rvagg mentioned this pull request Oct 21, 2015
@MylesBorins
Copy link
Contributor

should be considered for LTS

/cc @jasnell

@jasnell
Copy link
Member

jasnell commented Oct 24, 2015

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
Copy link
Member

jasnell commented Oct 30, 2015

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>
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