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

Made webpack respect NODE_PATH environment variable #476

Closed
wants to merge 3 commits into from

Conversation

jimmyhmiller
Copy link
Contributor

This addresses #253. Nothing should change by default, but you are able to set your NODE_PATH environment variable if you want absolute path imports.

Test Plan

I tested this by changing import App from './App'; to import App from 'App'; in index.js. Without setting NODE_PATH run and build will now fail because it can't find the module. After setting the the NODE_PATH to `./template/src' the project both builds and runs.

I also generated a new project, which you can find here that uses this absolute import. I did the equivalent change in the tests directory and the tests pass.

Let me know if you have any questions or any suggestions.

@ghost
Copy link

ghost commented Aug 22, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@@ -20,5 +21,6 @@ module.exports = Object
env['process.env.' + key] = JSON.stringify(process.env[key]);
return env;
}, {
'process.env.NODE_ENV': NODE_ENV
'process.env.NODE_ENV': NODE_ENV,
'process.env.NODE_PATH': NODE_PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

env.js is only used for variables injected into the app.
Doesn't seem like it's useful to expose it to the path.

env.js is only for variables injected into the app.
@ghost ghost added the CLA Signed label Aug 22, 2016
@ghost
Copy link

ghost commented Aug 22, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@gaearon
Copy link
Contributor

gaearon commented Aug 23, 2016

It does not currently support multiple paths. Would that be something you want me to add in this PR?

Definitely, we want to have feature parity with how Browserify treats NODE_PATH (which is likely how Node treats it, but would be great if you could check the source).

@jimmyhmiller
Copy link
Contributor Author

I couldn't find exactly where browserify handles NODE_PATH, but this test seems to suggest they support NODE_PATH the way node does.

I've added support for multiple paths. I moved the logic to paths.js. I'm not sure if this is the right place, but it seemed to make the most sense so that I don't duplicate the logic in the prod and dev config.

I have not tested this on windows myself, but I did test on my mac that local paths resolve when build and testing.

@gaearon gaearon added this to the 0.3.0 milestone Aug 27, 2016
@ghost ghost added the CLA Signed label Aug 27, 2016
@jimmyhmiller
Copy link
Contributor Author

Glad to see this added to a milestone! Any changes that need to before made before it could be merged?

@gaearon gaearon modified the milestones: 0.4.0, 0.3.0 Sep 1, 2016
@ghost ghost added the CLA Signed label Sep 1, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

Can you please verify that newly added testing with Jest also works?

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

I cherry-picked this via afe25df, 6e94bd8, 82066ac.
Verified that Jest works and added a few tweaks in c71337f.
I will close the PR but I’ll make sure to credit this feature to you in the changelog.

Thank you so much!

@gaearon gaearon closed this Sep 2, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

Out in 0.4.0. Thanks again!

@amandapouget
Copy link

It is really confusing here how to use this feature. Could someone give an example of exactly what you need to enter into your command line to make the sample App run with this line:
import App from './App'
changed to:
import App from 'App'

Tried a lot of variations of things like:
NODE_PATH=./template/src && npm run test
But everything I can think of still has the sample test failing.

@gaearon
Copy link
Contributor

gaearon commented Sep 19, 2016

@mandysimon88

If you use Bash on OS X or Linux, this should work:

NODE_PATH=./src npm start
NODE_PATH=./src npm run build
NODE_PATH=./src npm test

If you use Cmd on Windows:

NODE_PATH=./src&&npm start
NODE_PATH=./src&&npm run build
NODE_PATH=./src&&npm test

Note that lack of whitespace on Windows is intentional.

Does this help?

@gaearon
Copy link
Contributor

gaearon commented Sep 19, 2016

(I understand it’s frustrating this feature isn’t documented. It was added as a stopgap measure so we’d prefer not to advertise it widely. Ideally we’ll figure out some different solution to this before 1.0.)

@amandapouget
Copy link

Yes, this was really helpful. We ended up trying about 20 different variants on NODE_PATH= ? before stumbling on the answer. I can understand your desire not to advertise widely, but seriously, relative paths are a pain… I’m bringing over 33k lines of code from a previous project based on angular, and faced with updating the paths of every single import in every file. You can imagine the task.

On Sep 19, 2016, at 7:00 PM, Dan Abramov notifications@github.com wrote:

@mandysimon88 https://github.com/mandysimon88
If you use Bash on OS X or Linux, this should work:

NODE_PATH=./src npm start
NODE_PATH=./src npm run build
NODE_PATH=./src npm test
If you use Cmd on Windows:

NODE_PATH=./src&&npm start
NODE_PATH=./src&&npm run build
NODE_PATH=./src&&npm test
Note that lack of whitespace on Windows is intentional.

Does this help?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #476 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AMQpjGydkWGZr2OBED9YU8Zzq5qh1F6Kks5qrxQMgaJpZM4JqOmu.

@gaearon
Copy link
Contributor

gaearon commented Sep 21, 2016

Oh I can imagine. :P
If you'd like to do a PR let's add this to Usage Guide.

markerikson added a commit to markerikson/project-minimek that referenced this pull request Nov 22, 2016
There's been numerous requests for Create-React-App to support having
imports resolved relative to the "src" folder.  The semi-documented
solution is to have a NODE_PATH environment variable, which will be
used in the resolution process.  It's apparently also possible to
specify that variable in a file named ".env".

References:

facebook/create-react-app#476
facebook/create-react-app#693
facebook/create-react-app#741
markerikson added a commit to markerikson/project-minimek that referenced this pull request Nov 22, 2016
There's been numerous requests for Create-React-App to support having
imports resolved relative to the "src" folder.  The semi-documented
solution is to have a NODE_PATH environment variable, which will be
used in the resolution process.  It's apparently also possible to
specify that variable in a file named ".env".

References:

facebook/create-react-app#476
facebook/create-react-app#693
facebook/create-react-app#741
@mileung
Copy link

mileung commented Mar 24, 2017

When I run NODE_PATH=./src npm start

import App from './components/App'; works

import App from 'src/components/App'; doesn't work

Am I missing something?

@jimmyhmiller
Copy link
Contributor Author

In order to make that import you would need to have NODE_PATH=. npm start.
Because you said ./src the import would just be components/App.

I personally recommend putting this in your package.json.

"start": "NODE_PATH=. react-scripts start",

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

4 participants