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

Properly resolve absolute path of exposed module. #1059

Closed
wants to merge 1 commit into from

Conversation

jmm
Copy link
Collaborator

@jmm jmm commented Jan 8, 2015

The change in #1033 is flawed. And the test coverage for requires is inadequate.

See #1039 (part 1). A broken bundle is generated when doing something like...

// bundle.js
browserify('./src/a')
  .require('./src/b');

// src/a.js
require('./b');

// src/b.js
// I'm the file being required

...because the code in #1033 essentially does...

this._expose[row.id] = '/somedir/src/b';

...which leads to a mismatch here...

https://github.com/substack/deps-sort/blob/c61e515015a0f8392c1b07530877084b6c411cb7/index.js#L72

...because there should be an entry index['/somedir/src/b.js'], but instead there's index['/somedir/src/b'].

I'm not 100% sure this pull request completely and correctly solves the problem for all cases, but it does solve it for the scenario in #1039 and is a step in the right direction. That this apparently used to work (according to @gasi in #1033) and that #1033 was merged as a fix underscores the need for better and more comprehensive tests around this functionality.

Previously this incorrectly resolved the path of an exposed module,
resulting in broken bundles in cases like b.require('./somefile') where
there is a ./somefile.js that is also in the dependency graph of an
entry file.
@jmm
Copy link
Collaborator Author

jmm commented Jan 8, 2015

Here's an example of a test that currently fails:

jmm/node-browserify@ae9aedb

@ghost
Copy link

ghost commented Jan 15, 2015

Merged in 8.1.1 with an asynchronous version. Thanks for the patch!

@ghost ghost closed this Jan 15, 2015
@jmm
Copy link
Collaborator Author

jmm commented Jan 15, 2015

@substack Thanks! I started working on an async version after I submitted this pull request and I think it was shaping up similar to what you did (although I've only glanced at that so far) -- I was mimicking the pattern used for adding transforms -- but then I realized I think there's an even better solution, since the paths are already being resolved later on. I'll flesh that out a bit more and submit a pull request so you can see what you think.

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