Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

V3.0 flatten - may cause an issue with require() in existing code #8743

Closed
BorisKozo opened this issue Jun 28, 2015 · 4 comments
Closed

V3.0 flatten - may cause an issue with require() in existing code #8743

BorisKozo opened this issue Jun 28, 2015 · 4 comments
Milestone

Comments

@BorisKozo
Copy link

I know this was discussed before and probably the same problem exists with npm dedupe but since this is going to be a default behavior in v3.0 I thought to bring it up again.

The problem may manifest itself in two ways in existing code:

1 - Lets say I have a dependency in modules A and B, and module A has a dependency in B and module B exports an object with the method foo. The problem will occur if module A decides to override foo on module B. In v2 my code will run the original function foo of B as expected, with v3 I will be running the overridden function by A.

my code:

var a = require('A');
var b = require('B');

b.foo();

A code

var b = require('B');
b.foo = function(){ ... };

2 - One of my modules keeps a key value dictionary to keep track of some objects. The user of the module may add a key-value pair and later it is used for other stuff. In v2 each module/app would have its own dictionary and therefore key collision between modules would not happen. In v3 such collision may occur even between sibling modules. I will fix my module so that it doesn't use such logic (probably was a bad idea in the first place to do it) but not everyone will do so.

Some module A code

var myModule = require('myModule');
myModule.add('hello','world'); //key, value

Some module B code

var myModule = require('myModule');
myModule.add('hello','earth'); //key, value 

The execution of this code now depends on the order of how A and B are required in the code.

Since I am using Windows part of the time I am all for flattening but I think there should be a clear warning in the migration guide for this case.

@BorisKozo BorisKozo changed the title V3.0 flatten possible issue with singeltones V3.0 flatten - may cause an issue with require() in existing code Jun 29, 2015
@rmg
Copy link
Contributor

rmg commented Jul 1, 2015

This was always a problem if a module depended on myModule and one of its dependencies also depended on myModule. The deeper dependency would be met by the higher up version as long as the version was compatible.

A
 |- myModule@1.x
 |- foo
 |  |- myModule@1.x  <- would NOT be installed
 |- bar
    |- myModule@2.x <- would be installed

The difference with npm@3 is that it will actively seek out these opportunities where as npm@1 and npm@2 would merely allow for them if they happened.

@BorisKozo
Copy link
Author

As far as I know, the search algorithm will try to load the module from within the local node_modules first and only after it fails will try to load the one on top. Since most modules bring their own dependencies with them the problem will not happen in the common case.

Here are steps to reproduce the problem:
Create an empty project and install css-diff module. Then install colors module of the same version.

Now you have:

YourModule
             |
             |- css-diff
                    |
                    | - colors@0.6.1
             |- colors@0.6.1

In your module create a file called index.js with this:

var cssDiff = require('css-diff');
var colors = require('colors');

console.log(colors === cssDiff.colors);

Now go into css-diff module main file (index.js)
The first line there should be require('colors');
Go to the end of the file and add the line module.exports.colors = require('colors');

Run your index.js -> false (this is what happens with NPM 2)

Go to the node_modules of css-diff and delete colors from there.

Run your index.js -> true (this is what happens with NPM 3)

@rmg
Copy link
Contributor

rmg commented Jul 1, 2015

@BorisKozo that is correct from the perspective of now require() works. I was describing how npm@1 and npm@2 determines whether it needs to install a module.

In your example, if YourModule/node_modules/colors is already installed when npm is installing the dependencies for css-diff, then it will see that colors@0.6.1 is already met and it won't bother installing another copy of it because there is already one in the resolution path, relative to css-diff.

I've used this behaviour to remove duplicate dependencies by adding common sub-dependencies as top level dependencies and it can be quite effective.

@iarna iarna added this to the 3.x milestone Jul 1, 2015
@BorisKozo
Copy link
Author

@rmg Oh, I see what you mean. I didn't know this optimization exists during install.

So basically the same issue exists if say some internal module decides to upgrade one of its dependencies from version X to some version Y and version Y already installed at a higher level. Now installing all the modules again will not get the dependency locally at all and the code may break in the same way.

I hope that not many modules rely on local install or do monkey patching...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants