Uh oh!
There was an error while loading. Please reload this page.
Modularise scripts - #1433
Conversation
djgrant
commented
Jan 23, 2017
@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 |
djgrant
commented
Jan 26, 2017
I've introduced a convention of adding |
gaearon
commented
Feb 24, 2017
This looks pretty good. I'm sorry we missed it. |
24f5b35 to
4908921Comparedjgrant
commented
Feb 26, 2017
@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. |
Timer
commented
Mar 3, 2017
I'll review this. |
4908921 to
38bc01cCompareTimer
commented
Mar 3, 2017
| } | ||
| if (showInstructions) { | ||
| if (typeof onReadyCallback === 'function') { |
There was a problem hiding this comment.
It is confusing that we only call it when showInstructions is true.
We should instead pass it as an argument.
gaearon
commented
Mar 3, 2017
Looks good aside from one nit. |
Timer
commented
Mar 4, 2017
Before: After: Looks like we accidentally included |
Timer
commented
Mar 4, 2017
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. |
Timer
commented
Mar 4, 2017
AppVeyor CI green on my repo. |
djgrant
commented
Mar 4, 2017
| 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.
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.
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-ejectcomment, which will be skipped i.e.scripts/eject.js&scripts/utils/createJestConfig.js