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

Can't use NODE_PATH with two directories #3676

Closed
nareshbhatia opened this issue Jan 3, 2018 · 28 comments
Closed

Can't use NODE_PATH with two directories #3676

nareshbhatia opened this issue Jan 3, 2018 · 28 comments

Comments

@nareshbhatia
Copy link

Is this a bug report?

Yes

Can you also reproduce the problem with npm 4.x?

Did not try

Which terms did you search for in User Guide?

NODE_PATH, indexOf, undefined

Environment

  1. node -v: v8.7.0
  2. npm -v: 5.6.0
  3. yarn --version (if you use Yarn):
  4. npm ls react-scripts (if you haven’t ejected):

Then, specify:

  1. Operating system: MacOS
  2. Browser and version (if relevant): N/A

Steps to Reproduce

(Write your steps here:)

  1. Create a .env file in the root folder. Add NODE_PATH to it containing two directories: NODE_PATH=src:../shared
  2. Run npm start. You will get the following error:
/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/react-dev-utils/ModuleScopePlugin.js:28
        request.descriptionFileRoot.indexOf('/node_modules/') !== -1 ||
                                    ^

TypeError: Cannot read property 'indexOf' of undefined
    at Resolver.resolver.plugin (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/react-dev-utils/ModuleScopePlugin.js:28:37)
    at Resolver.applyPluginsAsyncSeriesBailResult1 (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/tapable/lib/Tapable.js:256:13)
    at runNormal (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/enhanced-resolve/lib/Resolver.js:130:20)
    at Resolver.doResolve (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/enhanced-resolve/lib/Resolver.js:116:3)
    at Resolver.<anonymous> (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/enhanced-resolve/lib/TryNextPlugin.js:16:12)
    at Resolver.applyPluginsAsyncSeriesBailResult1 (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/tapable/lib/Tapable.js:256:13)
    at runNormal (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/enhanced-resolve/lib/Resolver.js:130:20)
    at Resolver.doResolve (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/enhanced-resolve/lib/Resolver.js:116:3)
    at Resolver.<anonymous> (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/enhanced-resolve/lib/FileKindPlugin.js:17:12)
    at Resolver.applyPluginsAsyncSeriesBailResult1 (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/tapable/lib/Tapable.js:256:13)
    at runNormal (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/enhanced-resolve/lib/Resolver.js:130:20)
    at Resolver.doResolve (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/enhanced-resolve/lib/Resolver.js:116:3)
    at Resolver.<anonymous> (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/enhanced-resolve/lib/NextPlugin.js:14:12)
    at Resolver.applyPluginsAsyncSeriesBailResult1 (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/tapable/lib/Tapable.js:256:13)
    at runAfter (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/enhanced-resolve/lib/Resolver.js:152:20)
    at innerCallback (/Users/nbhati/projects/myapp-monorepo/myapp-embeds/node_modules/enhanced-resolve/lib/Resolver.js:146:3)

Expected Behavior

App is able to build correctly using packages from src and ../shared

Actual Behavior

Build crashes with TypeError: Cannot read property 'indexOf' of undefined

Reproducible Demo

None

@miraage
Copy link

miraage commented Jan 3, 2018

descriptionFileRoot is a directory of file being processed, managed by enhanced-resolve package (webpack dependency).

Source:

directory appears to be undefined for some reason.

AFAIK standard NodeJS (e.g. not installed by tools like nvm) provides its own NODE_PATH environment variable so you can not override it using .env files. I assume it might cause an issue, meanwhile I recall using custom NODE_PATH via .env (I'm using nvm which does not expose NODE_PATH env var) and everything worked as expected.

According to issue template, you have ejected your application. Could you please show output of process.env.NODE_ENV after loading env vars? E.g. after this line in scripts/starts.js

// Ensure environment variables are read.
require('../config/env');

@nareshbhatia
Copy link
Author

Hi @miraage, thanks for the quick response.

process.env.NODE_ENV is outputting development. You are correct that I have ejected. Note that NODE_PATH is working with one directory only: NODE_PATH=src. Also note that I am using n for installing multiple versions of node.

@miraage
Copy link

miraage commented Jan 3, 2018

Sorry for wrong question. I meant process.env.NODE_PATH :)
I'm not sure if n exposes NODE_PATH or not.

@Timer
Copy link
Contributor

Timer commented Jan 3, 2018

We intentionally do not support multiple directories in NODE_PATH. You can modify your ejected setup to allow this if you'd like.

@miraage
Copy link

miraage commented Jan 3, 2018

@Timer I always thought CRA supports multiple NODE_PATH dirs. I clearly recall using it..

@Timer
Copy link
Contributor

Timer commented Jan 3, 2018

Oh wow, my memory failed me! My apologies.

I'd heed @miraage's advice and check what the value of NODE_PATH is; it doesn't get overridden by .env -- .env only provides the default if it's not defined.

@miraage
Copy link

miraage commented Jan 3, 2018

@nareshbhatia are you using symlinks under the NODE_PATH? I guess it might affect path resolving in some way..

@nareshbhatia
Copy link
Author

Here's the value of NODE_PATH:

NODE_ENV= /Users/nbhati/projects/myapp-monorepo/myapp-embeds/src:/Users/nbhati/projects/myapp-monorepo/shared

I am not using any symlinks. The project is structured as follows:

myapp-monorepo
|--- myapp-embeds (CRA)
|    `--- src
`--- shared (shared components)

@miraage
Copy link

miraage commented Jan 3, 2018

There is also a line inside config/env.js after eject:

// Note that unlike in Node, only relative paths from NODE_PATH are honored.

@nareshbhatia could you please try to write NODE_PATH=./src:../shared? I assume src might cause an issue here.

@nareshbhatia
Copy link
Author

No change :-(. NODE_PATH=./src:../shared produces the same error.

If it helps, I can try to create a minimal app that reproduces this issue.

@miraage
Copy link

miraage commented Jan 3, 2018

Let's try.

@nareshbhatia
Copy link
Author

Ok, on it!

@nareshbhatia
Copy link
Author

@miraage, I have created a sample monorepo: https://github.com/nareshbhatia/cra-monorepo. Unfortunately I am not able to reproduce the original problem yet. The current problem is that CRA is not recognizing JSX components in the shared folder - this is even before ejecting. The good news is that it is able to reach components in the shared folder! The README file has further details.

If you can help me get past this issue, I can try to reproduce the original issue.

@miraage
Copy link

miraage commented Jan 3, 2018

The current problem is that CRA is not recognizing JSX components in the shared folder - this is even before ejecting

Correct. Only files under src directory will be processed by Babel.

I would seek for an issue by reviewing all post-eject configuration changes.

@nareshbhatia
Copy link
Author

@miraage, I ejected the app and made bunch of configuration changes:

  • Added an appShared export in paths.
  • Added paths.appShared to the webpack config so that the jsx files are passed through eslint and babel. See here and here.

This allowed the build to progress a bit further, but now something else is tripping up on shared/components/Profile.js, not recognizing it as a jsx file. Here's the error:

../shared/components/Profile.js
Syntax error: Unexpected token (5:16)

  3 | class Profile extends React.Component {
  4 |     render() {
> 5 |         return (<div>Profile</div>);
    |                 ^
  6 |     }
  7 | }
  8 |

@miraage
Copy link

miraage commented Jan 4, 2018

@nareshbhatia please, take a look at this plugin and comment above.

@nareshbhatia
Copy link
Author

@miraage, I completely removed the ModuleScopePlugin from webpack.config.dev.js, but the error still persists.

Unfortunately, I don't have more time to work on this issue. However, I think it is an important one to solve. I see that number of other people are concerned about it too (e.g. #1492). I hope that the CRA team will give it the priority that it deserves. Thanks again for your help.

@miraage
Copy link

miraage commented Jan 5, 2018

@nareshbhatia @Timer

I did a test. Fresh ejected CRA app.

Added shared path to config.paths.js:

➜  cra-3676 cat config/paths.js | grep appShared -A 2 -B 2
appPackageJson: resolveApp('package.json'),
appSrc: resolveApp('src'),
appShared: resolveApp('shared'),
yarnLockFile: resolveApp('yarn.lock'),
testsSetup: resolveApp('src/setupTests.js'),

Commented ModuleScopePlugin since it does not support multiple src directories.

➜  cra-3676 cat config/webpack.config.dev.js | grep "new ModuleScopePlugin"
// new ModuleScopePlugin(paths.appSrc, [paths.appPackageJson]),

Added shared to include section in eslint/babel loaders.

      {
        test: /\.(js|jsx|mjs)$/,
        enforce: 'pre',
        use: [
          {
            options: {
              formatter: eslintFormatter,
              eslintPath: require.resolve('eslint'),

            },
            loader: require.resolve('eslint-loader'),
          },
        ],
        include: [paths.appSrc, paths.appShared],
      },
          {
            test: /\.(js|jsx|mjs)$/,
            include: [paths.appSrc, paths.appShared],
            loader: require.resolve('babel-loader'),
            options: {

              // This is a feature of `babel-loader` for webpack (not Babel itself).
              // It enables caching results in ./node_modules/.cache/babel-loader/
              // directory for faster rebuilds.
              cacheDirectory: true,
            },
          },

Created a SharedComp component in shared folder:

➜  cra-3676 cat shared/SharedComp.js
import React from 'react';
import PropTypes from 'prop-types';

export class SharedComp extends React.Component {
    static propTypes = {
	text: PropTypes.string,
    };

    render() {
	return <div style={{margin: 10}}>{this.props.text}</div>;
    }
}

Added NODE_PATH:

➜  cra-3676 cat .env.local
NODE_PATH=./shared:./src

Updated src/App.js:

➜  cra-3676 cat src/App.js
import React, { Component } from 'react';
import logo from './logo.svg';
import './App.css';

import { SharedComp } from 'SharedComp';

class App extends Component {
  render() {
    return (
      <div className="App">
        <header className="App-header">
          <img src={logo} className="App-logo" alt="logo" />
          <h1 className="App-title">Welcome to React</h1>
        </header>
        <p className="App-intro">
          To get started, edit <code>src/App.js</code> and save to reload.
        </p>
        <SharedComp text="CRA 3676" />
      </div>
    );
  }
}

export default App;

Works like a charm: screen

Don't forget to update both dev/prod webpack configuration files.

@nareshbhatia
Copy link
Author

@miraage, thanks for the help.

Your solution works because src and shared are peers:

cra-3676
|--- src
`-- shared

I am looking for more separation so that two CRA apps can access shared components:

cra-monorepo
|--- cra-1
|      `--- src
|--- cra-2
|      `--- src
`-- shared

In this case, you will have to set NODE_PATH to ../shared:./src and appShared to ../shared. The moment you do this, CRA stops recognizing shared files as being JSX and starts throwing syntax errors. This is the use case I was trying to solve in my repo.

@miraage
Copy link

miraage commented Jan 6, 2018

@nareshbhatia you must mimic all actions I've provided. Then you'll have no problems at all.

@nareshbhatia
Copy link
Author

@miraage, I should have clarified - I tried all actions suggested by you both ways - once with src and shared being peers and then with shared at ../shared. Former worked, latter didn't. There is something about going up the tree that is tripping up the build.

@razum2um
Copy link

razum2um commented Sep 24, 2018

I'm about to extract some common code between 2 CRA's in a monorepo, and have the same issue: NODE_PATH=../shared:src in both CRAs makes eslint work, but webpack needs more includes for babel-loader

I wonder, why CRA supports multiple NODE_PATH, but webpack/babel-loader (at least) processes only appSrc, I think more logical would be similar approach: either 1 root or many in all places.

IMO described monorepo structure feels very natural, but instead, the current approach forces to have a real private npm package (preprocessed to es5 ofc) => which forces either setup a private registry or check in preprocessed es5 code

@Timer, btw, could you tell about #2189, because it's not self-explanatory, why did you do it

You can eject everytime

@miraage thank you for your efforts to show detailed way after eject in #3676 (comment), still I'd rather not, and given existence of "Alternatives to Ejecting" in readme, others don't want to eject as well ;)

p.s. I have no clear vision how this could be solved, but I can imagine some tools discovering their's configuration recursively when building sub-projects, wdyt about this?

p.p.s just in case: eslint is working after putting into .eslintrc.js:

require('dotenv').config({silent: true});
const path = require('path');
const paths = process.env.NODE_PATH.split(":").map(p => path.resolve(p));
exports.settings = {
  "import/resolver": {
    node: { paths }
  }
}

@Timer
Copy link
Contributor

Timer commented Sep 24, 2018

We don't compile source code outside of src/ so there's no sense in importing it, it's a no-operation.
We can't compile all source code because certain files need certain transforms.

@razum2um
Copy link

@Timer thanks for a fast reply, still, why support multiple NODE_PATH then? I see it misleading (worse with easy success with eslint): maybe warn that other entries should be either es5 or "please eject then"?

@nareshbhatia
Copy link
Author

@razum2um, I finally solved my code sharing issue using yarn-workspaces. You can read about my solution here: Sharing UI Components with Lerna and Yarn Workspaces.

@Timer
Copy link
Contributor

Timer commented Sep 24, 2018

NODE_PATH exists for absolute imports, some people will do src/components/ and src/containers/ and make sure there are unique names

@darksh3ll
Copy link

The react-scripts package provided by Create React App requires a dependency:

"eslint": "5.6.0"

@stale
Copy link

stale bot commented Nov 2, 2018

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs.

@stale stale bot added the stale label Nov 2, 2018
@Timer Timer closed this as completed Nov 2, 2018
@lock lock bot locked and limited conversation to collaborators Jan 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants