Fix importing npm linked libraries

Created by: AsaAyers

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

1. (#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:

> my-app@0.1.0 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.

Created by: gaearon

Does this also work with Jest?

Created by: 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.

Created by: gaearon

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

Created by: AsaAyers

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

Created by: gaearon

Need to review this. Tagging for next release.

Created by: 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.

Created by: 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.

Created by: Timer

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

@AsaAyers any reason you closed this?

Created by: gaearon

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

Created by: 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.

Created by: 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.

Created by: gaearon

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

Created by: 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.

Created by: Timer

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

Created by: Timer

Just tested this, thanks!

Merged by: Timer at 2017-03-23 00:29:53 UTC

Created by: gaearon

I think this fix was wrong in a strict sense. See https://github.com/facebookincubator/create-react-app/issues/3883.

Created by: gaearon

I’m reverting this in https://github.com/facebookincubator/create-react-app/pull/3884.