.env NODE_PATH is not working in v1.0.0

[CodinCat]

Description

I have NODE_PATH=src in .env, it did work great in v0.9.5. After yarn upgrade react-scripts it doesn't work now. And I didn't see any related guide in the v1.0.0 changelog.

(referenced from #2188)

I've also tried in a fresh new project and it doesn't work.

Expected behavior

Should be able to import 'foo/bar' which foo is a folder located in src

Actual behavior

Module not found: Can't resolve 'foo/bar' in '/Users/some/path/src'

Environment

Run these commands in the project folder and fill in their results:

1. npm ls react-scripts (if you haven’t ejected):
   v1.0.0
2. node -v:
   v7.1.0
3. npm -v:
   v3.10.9

Then, specify:

1. Operating system:
   macOS Sierra
2. Browser and version:
   Chrome 58

Reproducible Demo

https://github.com/CodinCat/cra-env-issue

This is a fresh new project. See /src/index.js:5

[ingro]

Didn't knew you could add that to .env, I'm using it in a custom npm script and it still works fine after updating to 1.0.0:

"scripts": {
  ...
  "dev": "cross-env NODE_PATH=./src npm run start"
}

I've got a problem with paths in tests by the way but I don't know if that's related to NODE_PATH or to the updated version of jest, I will report as soon as I could find where exactly the problem is.

[will-stone]

This is a real deal-breaker on upgrades to apps that use this form of importing. Changing an entire app to use relative paths will be tiresome. I tried @ingro's suggestion but that didn't work for me; still getting module not found errors.

[gaearon]

Interesting. Anyone looked yet into why this happens?

[RIP21]

Same for me. Have NODE_PATH=./ in .env and imports like src/components not working anymore.

[gaearon]

Thanks for confirming. Would be good if somebody could take a peek in the source into why. I'd expect it to work.

[Timer]

Seems unrelated to ModuleScopePlugin. Will investigate more.

[ingro]

As stated above I have only problems with tests, moving the __tests__ folder from the root to src (since now only tests inside src will run) returns me this weird error (my tests also relies on NODE_PATH):

 FAIL  src\__tests__\modules\stats\stats.test.js
  ● Test suite failed to run

    Cannot find module 'package.json'

      at Function.Module._resolveFilename (module.js:470:15)
      at Function.Module._load (module.js:418:25)
      at Module.require (module.js:498:17)
      at require (internal/module.js:20:19)

If I change imports in tests to be relative the error disappear.

This doesn't seem related to jest because I created a small repo to try to reproduce this error without cra but it's working fine there.

[gaearon]

Can you show the test code? Does it import package.json?

[Timer]

Looks like .env isn't being loaded at all, not that this feature doesn't work.
If I run NODE_ENV=src yarn start this works fine.

[gaearon]

I could've messed it up when refactoring something although I'd expect our E2E tests to catch that.

[Timer]

I don't believe any of our e2e tests use the .env file, only the feature itself.

[ingro]

Sure @gaearon , no package.json imported:

// src/__tests__/modules/statistiche/statistiche.test.js
import { actions } from 'modules/statistiche/actions/statistiche';

describe('Statistiche actions', () => {
    it('refreshAll action', () => {
        expect(actions.refreshAll()).toMatchSnapshot();
    });

    // ...
});

Also the imported file is pretty basic:

// src/modules/statistiche/actions/statistiche.js

const REFRESH_ALL   = 'statistiche/REFRESH_ALL';
// ...

function refreshAll() {
    return {
        type: REFRESH_ALL
    };
}

[gaearon]

@ingro We'd need a separate reproducing project to look into this. Not clear why.

@Timer I think I might know why it happens. env.js currently depends on paths.js which reads NODE_PATH before env.js could set it. 😄

[ingro]

Thanks @gaearon I'll try to create a repo to show the problem in the afternoon

[ApacheEx]

I agree with @gaearon, that's the reason. First we include paths.js where nodePaths is initialized as [] and then read .env files.

[Timer]

@gaearon this is karma for #2034 😆

[gaearon]

Lol. I guess we should just duplicate that resolving code in env.js. Since the normal "path resolving" can't apply until we know NODE_PATH.

[RIP21]

@ingro BTW I fixed my tests with cross-env so it looks like that now cross-env NODE_PATH=./ react-scripts test --env=jsdom. Ugly hack, but works for now. Waiting for fix :)

[ApacheEx]

in webpack config:

const getClientEnvironment = require('./env');
const paths = require('./paths');

the problem that paths.js will be cached when you include it in the second time. As solution we can remove cache after first reading in env.js

const paths = require('./paths');
delete require.cache[require.resolve('./paths')];

at least works for me.

[ingro]

Thanks for the suggestion @RIP21 but still not working for me, I was already running tests like this:

"scripts": {
   "test:cra": "react-scripts test --env=jsdom",
   "test": "cross-env NODE_PATH=./src npm run test:cra"
}

[ingro]

@gaearon I created a repo to reproduce the problem with tests. The tests are very silly but are there just to prove the point, the same test with relative paths will run while the other won't.

[gaearon]

The issue with Jest is unrelated, and caused by a different change in it.
I know the cause and will try to fix it today. The repro case by @ingro was very helpful.

[CodinCat]

Would you consider adding Webpack alias? I really think that NODE_PATH is just some kind of escape hatch to get rid of relative paths and might lead to some naming conflict. It would be really more convenient If we can have an alias.

I think the idea in Vue webpack template is great:
(https://github.com/vuejs-templates/webpack/blob/master/template/build/webpack.base.conf.js)

alias: {
  '@': resolve('src')
}

so you can

import Hello from '@/components/Hello'

and it's totally optional for users. Anyone who prefer relative paths can simply keep using them and ignore the alias. Of course it doesn't need to be @, this can be discussed.

[gaearon]

No, we don't plan to support non-standard extensions to Node resolution algorithm. This breaks every single tool that can operate on your projects. NODE_PATH is hacky but it embraces the fact that you are in fact doing something hacky 😉 . A better solution it to use symlinks in node_modules, but it has its own set of issues and incompatibilities. However, generally it works better than special configuration and conventions for every single tool.

[gaearon]

Okay, the fix for Jest is here: jestjs/jest#3616.
I’ll see if there’s any way to get in a bandaid fix in this repo until Jest fix gets into a release.

[gaearon]

react-scripts@1.0.1 should be out with a fix for both issues.
Can you please verify it helps?

[ingro]

Thanks @gaearon , now my tests are running again :D

[EddiG]

Looks like it still not working. In the https://github.com/CodinCat/cra-env-issue exist one App.test.js that testing only App.js, not index.js that contains issue. I have forked this repo into https://github.com/eddig/cra-env-issue.git, update react-scripts to 1.0.2 and fixed the foo/bar component requiring. Now error appears in App.js.
I think the problem with NODE_PATH appears because dotenv not override environment variables, that is already set (in my environment it is /usr/lib/nodejs:/usr/lib/node_modules:/usr/share/javascript).

Environment

• node --version
  v6.10.3
• yarn --version
  0.24.5
• Ubuntu 16.04 LTS

[ApacheEx]

@EddiG,
I can agree, it won't work if NODE_PATH is set in your system.
I found this: https://github.com/motdotla/dotenv#what-happens-to-environment-variables-that-were-already-set, so dotenv variables are skipped in your case.

I'd like to ask @gaearon
should we override environment variables that were already set?

[will-stone]

Please don't override already set variables. We set these on our Jenkins jobs and allow for each job to set up its own environment whilst keeping a .env file in the repo for development.

[ApacheEx]

@will-stone, I agree but it can be an option for file .env.override

[gaearon]

I think in this case the best way to do it is to put cross-env NODE_PATH=src before the scripts. As I said before this is a bit hacky anyway.

[RIP21]

@FezVrasta do you use react-app-rewired?
If so, it's not a problem of CRA. Cause it's known issue of it.

[tamsanh]

I never got cross-env working, nor any combination of using NODE_PATH. Instead, I put a package.json instead of my src folder that had the contents { "name": "src" }. This finally got it working.

src
├── Login
│   └── index.js
├── auth.js
└── package.json

$ cat src/package.json
{ "name": "src" }

$ head src/Login/index.js
import React, { Component } from 'react'
import auth from 'src/auth'
...

[RIP21]

@tamsanh lol. Interesting hack tho :D
Dunno, since NODE_PATH=./src in cross-env or .env works for me just fine.

[yarbshk]

I go in the following way (scripts/webpack.config.dev.js):

const paths = require('./paths');
module.exports = {
  resolve: {
    alias: { '@': paths.appSrc }
  }
};

[will-stone]

@yarbshk that requires ejecting. However I agree that using an "@" symbol is nice, and would put the tool on par with Vue; helping users coming from there.

[asadakbar]

@tamsanh I really like your approach because I really don't like having to define environment variables everywhere. But I hit an error after defining the package.json as you did, where src/Login/index.js couldnt be resolved when running yarn start. When running eslint, it seemed to assume that every import statement in any file under src was also a package and so threw a ton of errors. Any other details you can provide about how you got your solution working?

[gaearon]

However I agree that using an "@" symbol is nice, and would put the tool on par with Vue; helping users coming from there.

If this is valuable why don’t you file an issue for this? 🙂 Maintainers almost never read closed issues because there is too much noise. If feature requests happen in closed issues we won’t see your feature requests.

I’ll lock this issue because it is going off-topic. You are welcome to file more specific issues with your problems and suggestions, but using an old bug for a completely unrelated discussion just serves to confuse future readers coming here from Google etc.

Thanks!