Add an explicit root to webpack.resolve so it searchs there first

[jimmyhmiller]

This still needs (a lot) more work before it could be merged, but seems like a promising solution to #1023.

[jimmyhmiller]

I might have more time to work on this tomorrow. If not I for sure will Sunday. But anyone can feel free take it if that isn't soon enough.

[godmar]

Two suggestions:

• update the comments in the resolve section to reflect issue npm run build fails for EventEmitter #1023
• add an explicit 'modulesDirectories' setting as discussed in npm run build fails for EventEmitter #1023 - right now, it's subtly relying on the default settings for modulesDirectories including 'node_modules'

[jimmyhmiller]

I've added a comment to explain why this change was made and point to #1023. I'm not sure about changing moduleDirectories. Unless we are going to set it to some value other than the default, it doesn't make sense to me to explicitly set it.

I've tested the scenario listed in #1023 and the problem seems fixed. I've also tested to make sure local files don't shadow node_modules (npm install redux and have a file named redux.js). I've tested that you can still use NODE_PATHS to allow absolute imports. Everything seems to check out.

[godmar]

The reason I would explicitly set modulesDirectory is not only because the lack of the setting confused me until I discovered the default setting (which is hidden in the fine print of the webpack documentation), but because others were confused by this as well. For instance, the author of the first version of PR #476 was apparently confused as well about the role of root vs. fallback, leading to a patch that then had to be corrected. Perhaps for the same reason. ... I just noticed that was you. So, I'm not saying you were confused, but I certainly was. To debug the issue in #1023, I grepped for root and fallback and found the plugins I posted in the other thread, here. I briefly looked at modulesDirectories, but the plug-in code seemed to assume a non-empty setting, whereas we didn't have one. The default settings are applied elsewhere. Had I seen the setting, I would have found the problem much earlier because it would have provided an explanation for why installing events locally worked.

If it explicitly specified root + modulesDirectories + fallback, the full resolution order would be clear and those issues might be easier to debug in the future. Interestingly, that's what they appear to be doing for webpack 2.0. But, with the speed everything moves, they'll probably have upgraded to wp 2.0 before this PR lands. (Just kidding.)

[gaearon]

This directly contradicts the comment right below about how fallback works. If that comment is wrong, and our understanding of Webpack mechanism was wrong, then we should fix that too.

[gaearon]

It's not clear to me how this works. If the comment right below is wrong let's fix it. Otherwise this is inconsistent.

[jimmyhmiller]

@gaearon Not sure I see the contradict, but I'm sure it could be more clear.

root is the first placed that webpack looks for modules. Then it looks in modulesDirectories. Then it looks in fallback.

So, here's how the event problem was getting resolved:

First webpack checked root for event, it was not set, so it moved on. Then it checked node_modules and web_modules in the current directory (the users application node_modules) and all its ancestors. It didn't find it, so it check fallback. Fallback is using NODE_PATH so it checked there and found it.

This fixes that problem by specifying root as create-react-app's node_modules.

Hopefully that made sense. Happy to change the comment in any way to make that clear.

[godmar]

@jimmyhmiller almost correct.

It didn't "check node_modules and web_modules," it checked first web_modules and then node_modules because these are default settings for modulesDirectories

If we listed them, this would be more clear.

What's wrong about your comment is:

Without this, if NODE_PATH has the same module, webpack will use it before the dependency specified in our node modules.

where 'this' refers to root. It's wrong because without a root, webpack might still not use NODE_PATH if the module exists in node_modules.

I was wrong

[gaearon]

Apologies, I misunderstood the change.
I agree it would be clearer if we specified modulesDirectories too.

And if we listed those three settings together and wrote a single block describing the order webpack uses and why we picked those particular settings for each of the three keys.

[gaearon]

Please specify explicit modulesDirectories, consolidate three settings and unify their explanations into a single comment.

[jimmyhmiller]

@godmar You seem to understand this a bit better than me. If you'd be willing to write up a comment on it, I'd be grateful. If not, I can certainly take a stab at it.

[godmar]

How about:

    // Specify resolution order as follows:
    // 1. (root): create-react-app's node_modules folder. This ensures that
    //    packages provided by create-react-app take precedence, such as the
    //    browser shims from the node-libs-browser package.
    //    See https://github.com/facebookincubator/create-react-app/issues/1023
    root: paths.ownNodeModules,

    // 2. (modulesDirectories): Modules installed in the application's
    //    node_modules folders.  For clarity, we repeat the webpack 1.x default 
    //    for this setting.
    modulesDirectories: ['web_modules', 'node_modules'],

    // 3. (fallback): Some builds rely on being able to include packages via
    //    the folders listed in the (deprecated) NODE_PATH variable.  For 
    //    compatibility, we support such builds via fallback.  See
    //    https://github.com/facebookincubator/create-react-app/issues/253
    //
    //    Note that as of node 7.0.0 (Dec 2016) some Linux node.js packages
    //    provide a system-wide setting of NODE_PATH that refers to the
    //    directories in which node.js is installed.  Packages found there
    //    generally are not suitable for inclusion in a build and must be
    //    resolved via appropriate replacement packages in root or 
    //    modulesDirectories
    fallback: paths.nodePaths

The paragraph (Note that as of ...) is elucidatory only and could be elided.

[gaearon]

Would this root setting still make sense after the project is ejected?
Would it make any difference in that case?

[gaearon]

I don't understand what is the directory structure in which it would matter that paths.ownNodeModules is a root when node_modules exists in modulesDirectories.

How come the package exists in absolute root (likely node_modules/react-scripts/node_modules?) but not in node_modules search path? Is it being imported from somewhere other than node_modules/react-scripts/node_modules/node-browser-libs?

In other words: why does the regular resolution mechanism via modulesDirectories fail and we fall through to the fallback? Why doesn't it find the module in node_modules/react-scripts/node_modules/ just by traversing node_modules folders upwards?

[gaearon]

Understanding this is my only blocker to getting this merged.
(And I also want to make sure the configuration is sensible after ejecting.)

[godmar]

You asked:

Why doesn't it find the module in node_modules/react-scripts/node_modules/ just by traversing node_modules folders upwards?

My understanding is that the dependency arose at the top-level (say in src/index.js), so looking in node_modules/react-scripts/node_modules would be looking downwards, not upwards. We need to tell it to explicitly look there.

This is different, say, from dependencies that arose from packages within node_modules/react-scripts/node_modules.

Let @sokra clarify.

[gaearon]

My understanding is that the dependency arose at the top-level (say in src/index.js)

This is the part I don't understand. If things required by node-browser-libs get depended on from the top-level (e.g from generated code) then wouldn't the code crash completely without the NODE_PATH fallback? Why does it work on OS X?

[godmar]

Agreed, I don't understand this either. I suspected there was some something undocumented (?) in the resolution chain after root, modulesDirectories, and fallback.

[jimmyhmiller]

On my mac, the module resolves to myapp/node_modules/events/events.js. Does that file exist for you on linux @godmar?

[henri-hulski]

Note that as of node 7.0.0 (Dec 2016) some Linux node.js packages provide a system-wide setting of NODE_PATH

This is also true at least for node 5 and 6 so better leave it out.

[godmar]

@jimmyhmiller 'event' can resolve to myapp/node_modules/events/events.js iff you installed it there, or so I thought.

On my Linux box, I don't have that file in the CRA default configuration. In fact, doing a manual 'npm install events -S' put it there, and that was a work-around whose working we explained by the fact that the default setting of modulesDirectories included node_modules.

So... is it working on Macs because somehow there npm installs the browser shims in myapp/node_modules rather than in myapp/node_modules/react-scripts/node_modules - can you check where the other browser shims are?

[gaearon]

On my Linux box, I don't have that file in the CRA default configuration

Does this mean that the fix here doesn't help unless one also installs events (or similar shims) explicitly in that folder?

[gaearon]

Also, can the underlying problem still be reliably reproduced with 0.8.x?

[godmar]

@gaearon

Does this mean that the fix here doesn't help unless one also installs events (or similar shims) explicitly in that folder?

No. This proposed patch does not require installing events at the top-level.

[gaearon]

One thing that worries me about this fix is: what happens if user depends on something that react-scripts also depends on? Then we would give the user the potentially wrong version instead of the one they have in package.json.

We could also hardcode two roots (app's modules, then own modules) but it feels like we have no idea what's going on.

The better question to answer is how what imports these modules and why it can't find them by traversing parent directories, and instead goes to the fallback.

[jimmyhmiller]

The better question to answer is how what imports these modules and why it can't find them by traversing parent directories, and instead goes to the fallback.

From the documentation, modulesDirectories does not look in the parents of any folders only in the ancestors.

The reason this has different behavior on linux vs mac is because npm seems to resolve where to put the events dependency differently. I'm guessing this has to do with some sort of conflict triggering npm to use the old npm2 behavior.

[godmar]

I think the Macintosh people need to answer the question why it works for them; in particular, if it turns out that npm on their machines installs all packages upon which node-libs-browser depends in myapp/node_modules, then we have an explanation for why it works there even though root is not set.

Another option, of course, would be:

fallback : [...paths.ownNodeModules, ...paths.nodePaths]

(or paths.ownNodeModules.concat(paths.nodePaths) or whatever the correct syntax is.)

[jimmyhmiller]

I pushed a repo with my node_modules for the beginning create-react-app project. Just so we can have a comparison of mac vs linux.

https://github.com/jimmyhmiller/debug-create-react-app-deps

[henri-hulski]

@jimmyhmiller
Is this a clean cra install? They even have eventemitter3 installed.
I see this is react-scripts 0.8.1.
Can you reproduce the issue with this version?
Maybe add some information about your system.

[jimmyhmiller]

It is a clean install.

If by this version you mean master, yes I can reproduce it with master.

macOS: 10.12
node: v6.1.0
npm: 3.8.6

Happy to try any configuration people might want to see.

[henri-hulski]

0.8.1 is the version in package.json

[jimmyhmiller]

@henri-hulski Sorry I'm probably being dense. You are right that it is version 0.8.1 in the package.json. That seems to be the latest version. That being said, this repo was not created from master, but from the create-react-app installed globally.

Do you want me to run this from master? Do I need to update?

(Sorry this PR is getting full of comments)

[henri-hulski]

@jimmyhmiller The original issue was with version 0.7 and @gaearon asked

Also, can the underlying problem still be reliably reproduced with 0.8.x?

So I thought maybe you can answer this question.
But as you are on macOS you actually don't have this issue, right?

[godmar]

Linux (with NODE_PATH=/usr/lib/nodejs:/usr/lib/node_modules:/usr/share/javascript), node 6.9.1, npm 4.0.3, react-scripts 0.8.1.

'events' is present in myapp/node_modules/events and so import EventEmitter from "events" works.

import timers from 'timers'

still fails with:

SyntaxError: Name expected [/usr/lib/nodejs/buffer.js:5,0]

The correct resolution would be node_modules/node-libs-browser/mock/buffer.js from what I can see.

None of the proposed alternatives in this PR (setting root + modulesDirectory + fallback, or as I suggested setting only fallback to ownNodeModules + nodePaths) fixes the issue.

This is not too surprising as the directory ownNodeModules (node_modules/react-scripts/node_modules) is empty and thus can hardly contribute to a successful resolution process.

Yet, env NODE_PATH= npm run build still completes without attempting to include buffer.js in the build... but I can't find mock/buffer.js in the build (it should show a string Buffer not included).
This makes me think that it may not work on Macintosh, either, but rather crash at runtime.

We need a test that actually uses the referenced package.

[henri-hulski]

@godmar So when you remove NODE_PATH import timers from 'timers' still fails?

[godmar]

No, when I unset NODE_PATH (which is what env NODE_PATH= npm run build does), then the build completes without errors, however, I suspect that the build would crash at runtime if it actually used Buffer. By crash, I mean that 'Buffer' would be undefined. I base this suspicion on my not being able to find an actual code fragment that implements "Buffer" in the code, and I most certainly didn't find mock/buffer.js, which has a distinguishing string literal that ought to be in the build. (I may be wrong). Hence my suggestion to actually test the code that is built, rather than observing whether the build completes or not.

[gaearon]

Can you create two bundles, one with NODE_PATH unset and one normal, and post them so we can compare output byte by byte?

[godmar]

Who, me? If NODE_PATH is set, I don't get a bundle if I import timers.

[gaearon]

Oh right. I think this is the most confusing issue I have encountered for a long time. I'll try to reproduce it on some Linux machine tomorrow.

[gaearon]

Could it be that NODE_PATH matters not because we use it as fallback but because Webpack itself happens to resolve some module during compilation. And uses require.resolve or something like this to determine the path to the shim? Would resetting process.env.NODE_PATH right after reading it fix the issue?

[gaearon]

Thank you very much for your help investigating this.
I went with a different fix but your effort made it possible!
#1194

[godmar]

you're welcome.