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

Resolve app paths from realpath of CWD, fix #637 #648

Merged
merged 2 commits into from
Sep 16, 2016

Conversation

motiz88
Copy link
Contributor

@motiz88 motiz88 commented Sep 14, 2016

This makes all paths returned from resolveApp() begin with the realpath of the app root, resolving any symlinks etc.

Note: realpathSync throws when its argument is a non-existing path, so this change only assumes that process.cwd() exists and does a plain resolve from there.

@guivho Can you try this in your newly created project? npm install motiz88/create-react-app#resolve-symlink-cwd && npm start (later you can do npm install react-scripts to restore things to normal).

@ghost ghost added the CLA Signed label Sep 14, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 14, 2016

We also use require.resolve in a lot of places. Does it not have this issue?

@motiz88
Copy link
Contributor Author

motiz88 commented Sep 14, 2016

@gaearon:

  1. At least on Windows with Node 6, require.resolve does seem to return realpaths already (makes sense so as to not load modules more than once), so the simple answer is that no, it doesn't have this issue.
  2. All the uses of require.resolve that are currently in the code are safe anyway, in that I don't know of any problems that would be caused even if it did somehow return a symlink in those places.

Basically the only place where it matters if we alias the project dir is the Webpack config, and even that is arguably a Webpack bug (webpack/webpack#1643). I opted to implement this centrally in resolveApp because it was already there and it felt nicer than sprinkling fs.realpathSync all over the two Webpack config files.

@ghost ghost added the CLA Signed label Sep 14, 2016
@gaearon gaearon added this to the 0.4.2 milestone Sep 16, 2016
@gaearon gaearon merged commit 07105bf into facebook:master Sep 16, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 16, 2016

Thank you so much for tracking this down.

@gaearon
Copy link
Contributor

gaearon commented Sep 18, 2016

For the record, this was added to fix #637.

@gaearon gaearon mentioned this pull request Sep 18, 2016
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
* Resolve app paths from realpath of CWD, fix facebook#637

* Use new resolveApp() for NODE_PATH too
@lock lock bot locked and limited conversation to collaborators Jan 22, 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.

2 participants