-
-
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
Modularise scripts #1433
Modularise scripts #1433
Conversation
@gaearon Thinking that the dev server config could just be moved to |
This currently breaks CI after ejecting: https://travis-ci.org/facebookincubator/create-react-app/jobs/194654920 I think you might need to add more files to the copying script in |
I've introduced a convention of adding |
This looks pretty good. I'm sorry we missed it. |
24f5b35
to
4908921
Compare
@gaearon no worries. Now rebased. |
Is this functionally equivalent? If so, this looks great -- I feel like this is something we need to have rebased and just merged immediately to prevent merge issues one after another. My only concern is if this works with our recently-added support of linking react-scripts; but that's something I'd be willing to test after getting this in. Only reason I asked is because of the new method of getting files to eject. @gaearon, if this is rebased again can we merge immediately? I don't foresee any more script changes we're making in the 0.9.x branch. |
I'm fine with merging now but let's make sure we don't lose any recent changes accidentally. That is, the reviewer needs to actually read through those functions and make sure the contents match. |
I'll review this. |
4908921
to
38bc01c
Compare
} | ||
|
||
if (showInstructions) { | ||
if (typeof onReadyCallback === 'function') { |
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.
It is confusing that we only call it when showInstructions
is true
.
We should instead pass it as an argument.
Looks good aside from one nit. |
Before:
After:
Looks like we accidentally included |
Thanks so much for this, @djgrant! Your efforts will help many people who maintain forks of I hope I didn't upset you by making a few changes, we really wanted to get this in so we stopped breaking it every few days. Merging once CI is green. |
AppVeyor CI green on my repo. |
path.join('scripts', 'test.js') | ||
]; | ||
// Make shallow array of files paths | ||
var files = folders.reduce(function (files, folder) { |
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.
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.
We used to have a lower node requirement than Node 4, iirc. I'm not sure on an official preference.
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.
Arrows are fine. If it runs on Node 4 then it's good. But only in react-scripts
.
Global CLI should stay parseable by Node 0.12 so that we can show a nice error message instead of crashing. (We could also split modern code into a lazy require.)
* Refactor start script into modules * Move dev server config into config file * Replace eject file whitelist with a "remove-file-on-eject" flag * Move utils into scripts folder (for inclusion in ejection) * Add missed changes * Pass showInstructions as an argument * Fix eject bug * Don't eject babelTransform # Conflicts: # packages/react-cy-scripts/scripts/eject.js # packages/react-cy-scripts/scripts/start.js # packages/react-cy-scripts/utils/createJestConfig.js # packages/react-scripts/scripts/utils/createJestConfig.js # packages/react-scripts/utils/createJestConfig.js
* Refactor start script into modules * Move dev server config into config file * Replace eject file whitelist with a "remove-file-on-eject" flag * Move utils into scripts folder (for inclusion in ejection) * Add missed changes * Pass showInstructions as an argument * Fix eject bug * Don't eject babelTransform
Following the discussion and for reasons mentioned in #1431, this PR aims to refactor the webpack abstractions found in the start script out into their own utility modules.
I've aimed to maintain the general narrative of the script by keeping the running of methods in the script and using callback to eliminate interweaving.
Summary of changes
// @remove-file-on-eject
comment, which will be skipped i.e.scripts/eject.js
&scripts/utils/createJestConfig.js