Skip to content

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

Merged
gaearon merged 2 commits into
react:masterfrom
motiz88:resolve-symlink-cwd
Sep 16, 2016
Merged

Resolve app paths from realpath of CWD, fix #637#648
gaearon merged 2 commits into
react:masterfrom
motiz88:resolve-symlink-cwd

Conversation

@motiz88

Copy link
Copy Markdown
Contributor

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

@ghostghost added the CLA Signed label Sep 14, 2016
@gaearon

Copy link
Copy Markdown
Contributor

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

@motiz88

Copy link
Copy Markdown
ContributorAuthor

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

@ghostghost added the CLA Signed label Sep 14, 2016
@gaearongaearon added this to the 0.4.2 milestone Sep 16, 2016
@gaearon
gaearon merged commit 07105bf into react:masterSep 16, 2016
@gaearon

Copy link
Copy Markdown
Contributor

Thank you so much for tracking this down.

@gaearon

Copy link
Copy Markdown
Contributor

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

@gaearongaearon 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, fixreact#637
* Use new resolveApp() for NODE_PATH too
@locklockBot locked and limited conversation to collaborators Jan 22, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@motiz88@gaearon