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

Don't load same module multiple times if resolved from differnt paths #713

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@mantoni
Copy link
Contributor

mantoni commented Apr 2, 2014

There was an optimization which compared the dependency paths instead of
the hashes if the same modules where compared multiple times. This was
not taking into account that a module might be referenced from multiple
paths.

Removing the optimization solves the issue and does not seem to cause
any noticable performance drops.

Don't load same module multiple times if resolved from differnt paths
There was an optimization which compared the dependency paths instead of
the hashes if the same modules where compared multiple times. This was
not taking into account that a module might be referenced from multiple
paths.

Removing the optimization solves the issue and does not seem to cause
any noticable performance drops.

- Fixes issue #692
@substack

This comment has been minimized.

Copy link
Collaborator

substack commented Apr 6, 2014

Thanks for the patch! Merged in 3.38.1.

@mmckegg

This comment has been minimized.

Copy link

mmckegg commented Apr 19, 2014

Unfortunately this has broken the following case:

Module requires two copies of the same module (identical apart from path) and the sub module has a circular require.

https://github.com/mmckegg/browserify-bug-713

This is the case with readable-stream. If two different modules depend on the same version readable-stream (and no npm dedupe), then both of those modules are required in the same project, browserify throws a RangeError: Maximum call stack size exceeded

See https://github.com/isaacs/readable-stream/blob/master/lib/_stream_writable.js#L134 and https://github.com/isaacs/readable-stream/blob/master/lib/_stream_duplex.js#L44

/cc nodejs/readable-stream#88

mmckegg added a commit to mmckegg/loop-drop-app that referenced this pull request Apr 19, 2014

pin browserify at 3.37.2
until regression from browserify/browserify#713 is resolved

@mmckegg mmckegg referenced this pull request Apr 20, 2014

Closed

Max stack errors #735

@substack

This comment has been minimized.

Copy link
Collaborator

substack commented Apr 20, 2014

@mmckegg Thanks for putting in the work to isolate this bug! I'll include your test cases in the unit tests and have a patch out soon.

@substack

This comment has been minimized.

Copy link
Collaborator

substack commented Apr 20, 2014

Fixed in 3.44.1 and included @mmckegg's failing case in the tests.

@mantoni can you try 3.44.1 to see if it still works for your use case?

@mantoni

This comment has been minimized.

Copy link
Contributor Author

mantoni commented Apr 20, 2014

Can't verify because I'm not at work the next 2 weeks. Will check when I'm back. From looking at the diff it seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment