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

Resolve pathnames of exposed modules more efficiently. #1077

Merged
2 commits merged into from
Feb 21, 2015

Conversation

jmm
Copy link
Collaborator

@jmm jmm commented Jan 17, 2015

This is a followup to #1059. After I submitted that, I was working on an async version, which I didn't have ready yet when you pushed your async implementation, @substack. Then I realized that I don't think it's necessary to even do the resolution in browserify, because it looks like module-deps already does it, before anything else needs access to it.

This changeset relies on related changes to module-deps (browserify/module-deps/pull/65).

This may have some rough edges that need to be smoothed out.

  • This currently falls down when doing:
b.require('./somefile');

Whereas this works:

b.require('./somefile', {expose: 'whatever'});

In the former case it leads module-deps to mark the file as builtin which causes transforms not to be run on it. I didn't know that, because tests were passing. I didn't discover it until debugging #1074. One solution would be to set a different prop on the object in b.require(), e.g. exposeAs instead of expose, but that seems kind of silly. I haven't had the chance to do it yet, but I think it would be good to take a closer look at how module-deps is handling that scenario and see if there isn't a correction needed there.

jmm added 2 commits January 15, 2015 18:45
Set the stage for mdeps to assign the resolved pathnames for exposed
modules.
row.expose is for mdeps. It will be resolving the pathnames for the
files written to the pipeline here anyway. So instead of resolving the
pathnames here, just give mdeps the id to expose the module as and let
it assign the resolved path.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant