-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Conversation
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 |
There was a problem hiding this comment.
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.
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Definitely, we want to have feature parity with how Browserify treats |
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. |
Glad to see this added to a milestone! Any changes that need to before made before it could be merged? |
Can you please verify that newly added testing with Jest also works? |
Out in 0.4.0. Thanks again! |
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: Tried a lot of variations of things like: |
@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? |
(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.) |
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.
|
Oh I can imagine. :P |
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
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
When I run
Am I missing something? |
In order to make that import you would need to have I personally recommend putting this in your package.json.
|
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';
toimport App from 'App';
in index.js. Without settingNODE_PATH
run and build will now fail because it can't find the module. After setting the theNODE_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.