-
-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
add support for installing react-scripts using npm link #1122
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! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions. |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
EDIT: Fixed build issues. Apologies for the spam. |
add support for installing react-scripts via npm link
nit: grammar fix in comment
with this change, are the scoped package steps still needed if you're not making a long running fork? I want to be able to take my existing app, pull someone's branch for an open PR, run |
Yep, should work. Running |
// `publish`) or running the smoke test. | ||
var isRunningFromOwn = __dirname.indexOf(path.join('packages', 'react-scripts', 'config')) !== -1; | ||
var isSmokeTest = process.argv.some(arg => arg.indexOf('--smoke-test') > -1); | ||
var isRunningFromAppUsingLink = (process.env.npm_lifecycle_event === 'start') && isRunningFromOwn && !isSmokeTest |
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.
What is special about start
? I don't understand this.
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.
Without including the check for start
, (i.e. only checking isRunningFromOwn && !isSmokeTest
), the tests fail because those conditions are both true for other events like build
. At first glance, it may be assuming too much, as it's entirely possible that someone will write an app that has other start-like events, but are named something else. But they should be able to handle their own custom cases since they'll be forking react-scripts in the first place.
Could you write up another explanation about why this works and solves that use case? |
When you try to use
With the special logic to track |
Tagging as 0.9.1 as I'd like us to understand what's the problem here, and maybe merge this in some form. |
I believe that this is trying to accomplish what #1356 does, but in a much more convoluted way. I appreciate all your hard work and explanations @tabrezm, but I'm going to close this in favor of #1356 since it is a more straightforward modification. Please do not hesitate to reach out to us if this does not resolve your use case. |
When using npm link to depend on a forked version of react-scripts, the paths are set up incorrectly such that the source files in the react-scripts template get used instead of the actual source files from the app.
Repro steps:
create-react-app
.create-react-app
from GitHub and convertpackages/react-scripts
to a scoped package. For example, change this:to this:
npm link
frompackages/react-scripts
.npm link @tabrezm/react-scripts
from the folder for the package created in step 1. Also, change the following lines inpackage.json
from this:to this
App.js
and runnpm start
.Expected: see new content
Actual: see content from template in
react-scripts
insteadFull discussion: #682 (comment)