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

Symlink-friendly path resolution #277

Merged
merged 3 commits into from
Jul 29, 2016
Merged

Symlink-friendly path resolution #277

merged 3 commits into from
Jul 29, 2016

Conversation

dallonf
Copy link
Contributor

@dallonf dallonf commented Jul 29, 2016

I was having difficulties using a local copy of react-scripts and npm linking it into a real world project. This change resolves paths relative to the current working directory (that is, most likely the directory of the app) rather than assuming a certain directory structure.

I've tested the "happy path" (that is, the use case I was trying to solve) locally and solved the problem, both by just running npm start and npm run ejecting and then running the app again. I was unable to test the other case of installing from npm - I'm actually not quite sure how to verify that. I imagine that's pretty important though, so I'd be very open to suggestions and guidance! I did run npm test which seems to cover a lot of cases though.

I was having difficulties using a local copy of `react-scripts` and `npm link`ing it into a real world project. This change resolves paths relative to the current working directory (that is, most likely the directory of the app) rather than assuming a certain directory structure.
@facebook-github-bot
Copy link

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 [email protected]. Thanks!

@gaearon
Copy link
Contributor

gaearon commented Jul 29, 2016

I was having difficulties using a local copy of react-scripts and npm linking it into a real world project.

What was the use case? You can develop create-react-app locally by cloning it and running npm start or npm run build—all existing commands should work right there. You could drop your project’s source into template/src for experimentation.

Symlinks are generally hard to get right, and even if we fix something now, we might break it again in the future.

@ghost
Copy link

ghost commented Jul 29, 2016

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

@ghost ghost added the CLA Signed label Jul 29, 2016
@dallonf
Copy link
Contributor Author

dallonf commented Jul 29, 2016

I've been tasked with creating a React "boilerplate" for my company and this project seemed like a good starting point. But one thing we're going to need is the ability to make changes to the app locally and unblock ourselves if we run into a bug or missing feature, while it gets merged into the main repo.

As a more general use case, it would allow the same workflow for the main Facebook version of this project - if somebody encountered a bug while working on their real-world project, they could npm link react-scripts, submit a PR, and keep working while they wait. This seems to be a fairly standard JS/npm workflow... at least when it works.

I suppose another solution could be to attempt to symlink your project's source into template/src, sort of like you described (except that directly copying would make it somewhere between impossible and highly inconvenient to continue working and committing to your own project)... if that's the preferred solution, I'd be happy to send a docs PR for that.

appSrc: resolve('../template/src'),
appNodeModules: resolve('../node_modules'),
ownNodeModules: resolve('../node_modules')
appBuild: resolveLib('../build'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s call this resolveOwn.

@gaearon
Copy link
Contributor

gaearon commented Jul 29, 2016

I‘m open to merging this if it fixes the problem for you.
Just have one small nit.

@dallonf
Copy link
Contributor Author

dallonf commented Jul 29, 2016

Ah yes... naming things 👍

@gaearon gaearon added this to the 0.3.0 milestone Jul 29, 2016
@ghost ghost added the CLA Signed label Jul 29, 2016
@gaearon gaearon merged commit d2baa3c into facebook:master Jul 29, 2016
@gaearon gaearon modified the milestones: 0.2.1, 0.3.0 Aug 1, 2016
@gaearon gaearon mentioned this pull request Aug 1, 2016
@gaearon
Copy link
Contributor

gaearon commented Aug 1, 2016

Released in 0.2.1.

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

None yet

3 participants