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

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

Closed

Conversation

jimmyhmiller
Copy link
Contributor

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

@jimmyhmiller
Copy link
Contributor Author

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
Copy link

godmar commented Dec 2, 2016

Two suggestions:

@jimmyhmiller
Copy link
Contributor Author

jimmyhmiller commented Dec 4, 2016

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
Copy link

godmar commented Dec 4, 2016

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
Copy link
Contributor

gaearon commented Dec 5, 2016

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
gaearon previously requested changes Dec 5, 2016
Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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
Copy link

godmar commented Dec 5, 2016

@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.

@gaearon gaearon dismissed their stale review December 5, 2016 20:22

I was wrong

@gaearon
Copy link
Contributor

gaearon commented Dec 5, 2016

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.

Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@gaearon gaearon added this to the 0.8.2 milestone Dec 5, 2016
@jimmyhmiller
Copy link
Contributor Author

@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
Copy link

godmar commented Dec 6, 2016

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
Copy link
Contributor

gaearon commented Dec 6, 2016

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

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

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
Copy link
Contributor

gaearon commented Dec 6, 2016

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

@godmar
Copy link

godmar commented Dec 6, 2016

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
Copy link
Contributor

gaearon commented Dec 6, 2016

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
Copy link

godmar commented Dec 6, 2016

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

@jimmyhmiller
Copy link
Contributor Author

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

@henri-hulski
Copy link

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
Copy link

godmar commented Dec 6, 2016

@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
Copy link
Contributor

gaearon commented Dec 6, 2016

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
Copy link
Contributor

gaearon commented Dec 6, 2016

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

@godmar
Copy link

godmar commented Dec 6, 2016

@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
Copy link
Contributor

gaearon commented Dec 6, 2016

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
Copy link
Contributor Author

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
Copy link

godmar commented Dec 6, 2016

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
Copy link
Contributor Author

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
Copy link

@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
Copy link
Contributor Author

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
Copy link

0.8.1 is the version in package.json

@jimmyhmiller
Copy link
Contributor Author

@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
Copy link

@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
Copy link

godmar commented Dec 6, 2016

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
Copy link

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

@godmar
Copy link

godmar commented Dec 6, 2016

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
Copy link
Contributor

gaearon commented Dec 6, 2016

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
Copy link

godmar commented Dec 6, 2016

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

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

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
Copy link
Contributor

gaearon commented Dec 6, 2016

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
Copy link
Contributor

gaearon commented Dec 7, 2016

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

@gaearon gaearon closed this Dec 7, 2016
@godmar
Copy link

godmar commented Dec 7, 2016

you're welcome.

@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants