Fix importing npm linked libraries

[AsaAyers]

Looking through the npm link issues, I see two use cases:

1. (Fix workflow if react-scripts package is linked via npm-link #1356) using npm link to test react-scripts
2. using npm link for libraries your app depends on.

This PR solves the 2nd case. Webpack's official solution for npm link is to add your project's node_modules to resolve.fallback on the config.

To verify the issue and fix create a small library to link:

cd /tmp
git clone https://gist.github.com/f40e1bad454626691097b4cc4651208c.git link-example
cd link-example
npm link
# to clean this up when you're done, return and run `npm unlink`

Then create your app and link the library.

create-react-app my-app
cd my-app
npm link link-example
echo 'import "link-example"' > src/index.js
npm run build

With the current published version, the build will fail:

> [email protected] build /tmp/my-app
> react-scripts build

Creating an optimized production build...
Failed to compile.

Module not found: Error: Cannot resolve module 'react' in /tmp/link-example

Even though my-app imported ./node_modules/link-example/index.js, the symlink causes webpack to treat it like you required /tmp/link-example/index.js.

[gaearon]

I think it is slightly confusing to do it here, since this block is specifically for NODE_PATH.
Can we do this directly in Webpack configs instead?
It's fine to have some duplication in case you're worried about it.

[gaearon]

Does this also work with Jest?

[AsaAyers]

Jest doesn't need anything, it already handles symlinks correctly. I verified by updating my link-example to: module.exports = require('react'). Then I created a test file in my-app:

import React from 'react'
import React2 from 'link-example'

test('Import is the same', () => {
    expect(React2).toBe(React)
})

So I went ahead and moved my code. When I rebased I discovered that master already has my change in both files. webpack.conf.dev.js and webpack.config.prod.js.

I tried the test plan from this issue against master, and I the problem isn't actually fixed. My best guess is that this solution doesn't work with Webpack 2.

[gaearon]

The config format has changed in WP2, Could that be the reason?

[AsaAyers]

I found my problem. master uses node_modules, but it really needs to be the path found in paths.appNodeModules.

[gaearon]

Need to review this. Tagging for next release.

[Timer]

Remember that this format is completely different than that of webpack 1, see https://github.com/facebookincubator/create-react-app/blob/0.9.x/packages/react-scripts/config/webpack.config.dev.js#L79-L96.

Webpack migration docs said to specify 'node_modules' as the default, see https://webpack.js.org/guides/migrating/#resolve-root-resolve-fallback-resolve-modulesdirectories.

If we merge this, we need to set the root property in the 0.9.x branch. The current PR only fixes this for webpack 2.

[gaearon]

I understand, I just mean that we should review this as part of the next review cycle, and cherry-pick an equivalent (but maybe different code-wise) fix into 0.9.x if this gets in.

[Timer]

Sorry if I came off harsh. Just leaving that comment for future reference.

@AsaAyers any reason you closed this?

[gaearon]

Haha no worries, just explaining what I mean when I tag PRs because it's probably not super obvious with our branch setup

[AsaAyers]

We restructured the project that needed this and I'm not interested in figuring out the Webpack1 to Webpack2 conversion here. I completely forgot this was still open until you reminded me today.

[Timer]

@AsaAyers we still greatly appreciate your contribution and will handle the webpack 1 conversion for you. We'll most likely land this if it works correctly. 😄

[gaearon]

We'll likely take it over from here, thanks for sending the PR!

[AsaAyers]

Thanks. This passed the test plan in the body of the PR last time I worked on it.

Sorry for the small misunderstanding here. I didn't notice the first message about reviewing and tagging for release.

[Timer]

Do we actually want to get rid of "node_modules", or just add the appNodeModules as a second option?

[Timer]

Just tested this, thanks!

[gaearon]

I think this fix was wrong in a strict sense.
See #3883.

[gaearon]

I’m reverting this in #3884.