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

Feature request: Avoid loading linked modules multiple times #692

Closed
mantoni opened this Issue Mar 11, 2014 · 23 comments

Comments

Projects
None yet
@mantoni
Copy link
Contributor

mantoni commented Mar 11, 2014

We run into the following issue with quite a large dependency tree:

When linking packages with npm link that share a dependency, Browserify loads the module twice because they are in different paths. This leads to various problems from multiple socket connections to instanceof not working anymore.

It would be great if Browserify could consider the package.json's name / version to detect this case.

Thanks!

@thlorenz

This comment has been minimized.

Copy link
Collaborator

thlorenz commented Mar 12, 2014

Actually if the module has the exact same content it would be deduped by browserify.
Are the ones that get loaded different versions?

If that it the case, then you ran into an edgecase regarding npm dedupe. I tried to solve that with deli.
I stopped working on it for the time being, but possibly you can add some comments to its issues to help get it finished? At this point linking works, but unlinking doesn't.

@mantoni

This comment has been minimized.

Copy link
Contributor Author

mantoni commented Mar 12, 2014

I'm not using npm dedupe in this szenario. We have this:

  A
 / \
B   C
 \ /
  D

D is an application bundle where I'm pulling in different modules that make up the web application consisting of B and C. Both use the common library A.

I then npm link B and C into D for testing locally. Now A gets loaded twice, even if the version and content is exactly the same.

Of cause I could also link A into B and C to solve this, but we have this kind of problem with 5 modules already. Running dedupe or any other tool might help, but is inconvenient. Therefore this feature request.

@mantoni

This comment has been minimized.

Copy link
Contributor Author

mantoni commented Mar 15, 2014

You said that browserify dedupes packages with the same content. Can you point me to the place in the code where this is happening? I can't find it. I would like to understand the mechanism to see why it's not working for me.

@thlorenz

This comment has been minimized.

Copy link
Collaborator

thlorenz commented Mar 15, 2014

this should be a good starting place to investigate dedupe.

@mantoni

This comment has been minimized.

Copy link
Contributor Author

mantoni commented Mar 15, 2014

Thank you! I've run a test with some of my open source projects and can now reproduce the problem like this:

  • I've added console.log(dup._id, row.id) in line 511
  • When creating the bundle, I get some matches
  • For one of them, I added a console.log('A'); and a console.log('B'); statement in both files.
  • When I create the bundle again and run it, I see A and B being printed

So the dedupe algorithm seems to be working fine, but the module gets loaded twice nevertheless.
Any ideas where to look next?

@andreypopp

This comment has been minimized.

Copy link
Contributor

andreypopp commented Mar 15, 2014

@mantoni I think that's by design — two modules under different paths are considered different modules by module system and so they are evaluated twice

@thlorenz

This comment has been minimized.

Copy link
Collaborator

thlorenz commented Mar 15, 2014

@mantoni the fact that they get loaded/evaluated twice is not a problem. The important part is that they only get included in the bundle once to keep it small (granted they have the exact same content).

@mantoni

This comment has been minimized.

Copy link
Contributor Author

mantoni commented Mar 15, 2014

Yes. All good. I understand and even though there is more going on than I thought, it's actually behaving exactly the way I thought it is.

I'd still like to have a --dedupe switch or something in Browserify to make it load each module with the same content only once. Just as it does when I run dedupe before bundling.

It would help a lot in development if npm link could be used with Browserify.

@mantoni

This comment has been minimized.

Copy link
Contributor Author

mantoni commented Mar 16, 2014

As I read it, if two dependencies pass the sameDep test, the source is rewritten to simply module.exports=require('other-dep');.
How is it possible, that this still causes the module to be loaded twice?
I think I'm still misunderstanding something here.

@mantoni

This comment has been minimized.

Copy link
Contributor Author

mantoni commented Mar 16, 2014

Ok. I'm stupid. It's not loading it twice. Since I edited the two files that where marked as dupes with different console statements, they are not the same anymore and therefore get loaded twice.

I'll look into the actual issue I have at work tomorrow again with the new insights.

@mantoni

This comment has been minimized.

Copy link
Contributor Author

mantoni commented Mar 17, 2014

Works as advertised. Thank you guys for walking me through. Helped a lot to understand some of the browserify internals.

There was a different reason why things where going wrong on my side: I am using the dependency list from browserify to perform additional actions. I have to filter out dupes here myself, which makes perfect sense.

Thanks again. Closing.

@mantoni mantoni closed this Mar 17, 2014

@graue

This comment has been minimized.

Copy link

graue commented Mar 18, 2014

@thlorenz

"the fact that they get loaded/evaluated twice is not a problem."

I disagree with this statement :) I'm dealing with a similar issue, where that fact is exactly my problem.

I have a library that has code sort of like this:

var thingRegistry = {};
exports.addThing = function(key, thing) {
  thingRegistry[key] = thing;
}
exports.lookupThing = function(key) {
  return thingRegistry[key];
}

The library is required both directly and as a dependency of a second library. I want there to be only one global registry - but there are two. I can't seem to find a way around this other than saving the registry to window.

Do you really disagree that this is a bug? It seems like the module system's job to ensure a module doesn't get loaded multiple times.

@thlorenz

This comment has been minimized.

Copy link
Collaborator

thlorenz commented Mar 18, 2014

You are dealing with a problem that browserify cannot reliably solve for you.
You'd have the exact same problem in node once you use ln -s since you link outside of your node_modules dependency tree and as I outlined earlier I wrote deli to work around this issue.

The correct way to solve this though is to enforce a singleton by exposing it from one place like a core module.

Keep in mind that singletons are not guaranteed by either module loader (be it node or browserify). A lot of times you do get the same instance due to caching, but you shouldn't rely on that in order to enforce singleton semantics since it breaks in lots of cases as you have shown.

@mantoni

This comment has been minimized.

Copy link
Contributor Author

mantoni commented Mar 21, 2014

Turns out my problem is actually a bug in browserify:

sameDeps has an optimisation which uses deepEquals for visited nodes instead of hash comparison.
https://github.com/substack/node-browserify/blob/master/index.js#L626

The problem is, that the paths for the dependencies can be different, but the content is the same.
If I remove the optimisation, my problem disapears.

I can provide a patch, but the question is whether I should just take out the optimisation or come up with an alternative algorithm?

@mantoni mantoni reopened this Mar 21, 2014

@andreypopp

This comment has been minimized.

Copy link
Contributor

andreypopp commented Mar 21, 2014

I confirm, browserify doesn't dedupe, even if modules have the same content.

mantoni added a commit to mantoni/node-browserify that referenced this issue Apr 2, 2014

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 browserify#692

substack added a commit that referenced this issue Apr 6, 2014

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

fixed by #713

@substack substack closed this Apr 6, 2014

@madcapnmckay

This comment has been minimized.

Copy link

madcapnmckay commented Aug 6, 2015

@substack @mantoni - I think this bug may still be around. I had two versions of a common module being included even though it was the same version. I did a diff of the two directories and they actually differed because package.json has a _from property that denotes the version range that caused that module to be included. Because the deps were being included with a slightly different carart include i.e. ^1.3.2 & ^1.3.1 respectively, the _from field was different even though the actual module contents was identical.

Even after I fixed that issue and my manual diff passed two instances were still included.

Needless to say this is super brittle. Out of interest why can't browserify be version aware? Wouldn't that solve these issues and avoid having to do hash comparisons?

@substack

This comment has been minimized.

Copy link
Collaborator

substack commented Aug 6, 2015

browserify doesn't look at the package.json version field at all. If the file contents are different, browserify will include both versions. If the contents are identical, only one version is included.

Browserify doesn't look at the version field because it's unreliable. A package can have whatever value it wants for the version fields, but the contents of files may still differ. This is common if you're working on a package locally and haven't published an updated version to npm.

Another reason why you might have a shim version of a file for the same contents (but not the same exact contents twice) is that a package in a different location, even if it has been symlinked, will resolve to different files. For example, node_modules/foo will see a different value for require('bar') if node_modules/foo/bar/index.js exists versus the exact same contents or a symlink for foo where a different version of the bar dependency exists.

I recommend using npm dedupe to fix these issues or install npm v3: npm install -g npm@^3, which dedupes by default.

@pmowrer

This comment has been minimized.

Copy link

pmowrer commented Sep 17, 2015

@madcapnmckay You may want to try browserify-resolutions. It's a plugin to Browserify that more aggressively dedupes

@saiichihashimoto

This comment has been minimized.

Copy link

saiichihashimoto commented Oct 16, 2015

I've run into this same issue. In my case, the dependency looks more like:

  B
   \
B   C
 \ /
  D

where D is the application bundle, B is underscore, and D is npm linking C. In my case, when C requires B, it throws an error:

Uncaught TypeError: Cannot read property '0' of undefined

What I'm requiring also is going through the jstify transform, which may be part of the issue?

Regardless, to deal with this, in both C & D I npm link B. Then they happily use the same version.

This is... poor. Especially if there's more than one dependency that they share. This shouldn't be a manual fix like this.

@hugomallet

This comment has been minimized.

Copy link

hugomallet commented Mar 10, 2016

Hi,

Is there a fix planned for this issue?
Or is there a preferred development practice to dedupe dependencies of linked modules?

@ericsoco

This comment has been minimized.

Copy link

ericsoco commented Apr 6, 2016

Finally found a reusable solution to the problem of duplicated deps when npm linking, and it's not specific to any build tool:
http://stackoverflow.com/a/36464203/222356

In short, remove secondary dependencies from the npm linked module, add your downstream project's node_modules to NODE_PATH, and resolve secondary dependencies in your downstream project.

e.g. NODE_PATH=$(pwd)/node_modules/ npm start

Curious to hear your thoughts.

@oztune

This comment has been minimized.

Copy link

oztune commented May 18, 2016

I found the solution @ericsoco suggested to work perfectly without requiring any project changes.

For future readers, to make this fit into our existing workflow I put together a node module you may find useful and save you some keystrokes: https://github.com/oztune/gn

Hagith pushed a commit to AtendeSoftware/node-browserify that referenced this issue Jan 24, 2017

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