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

Modularise scripts #1433

Merged
merged 8 commits into from Mar 4, 2017

Conversation

Projects
None yet
5 participants
@djgrant
Copy link
Contributor

commented Jan 23, 2017

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

/config
  + webpackDevServer.config.js
/scripts
  /utils
     + addWebpackMiddleware.js
     ~ createJestConfig.js
     + createWebpackCompiler.js
  ~ eject.js
  ~ start.js 
  • All files found in specified folders get copied to the app directory on eject
  • Except files flagged with a // @remove-file-on-eject comment, which will be skipped i.e. scripts/eject.js & scripts/utils/createJestConfig.js
@djgrant

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2017

@gaearon Thinking that the dev server config could just be moved to config/ instead.

@gaearon

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

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 scripts/eject.js.

@djgrant

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2017

I've introduced a convention of adding // @remove-file-on-eject to the top of files that are not to be copied over at ejection. This removes the need for a whitelist of files and allows me to put the utils folder inside of scripts where it can be used in an ejected app.

@gaearon

This comment has been minimized.

Copy link
Member

commented Feb 24, 2017

This looks pretty good. I'm sorry we missed it.
Can you please rebase this with latest changes?

@gaearon gaearon added this to the 0.10.0 milestone Feb 24, 2017

@djgrant djgrant force-pushed the djgrant:enhancement/modularise-scripts branch from 24f5b35 to 4908921 Feb 26, 2017

@djgrant

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2017

@gaearon no worries. Now rebased.

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Mar 3, 2017

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.

@gaearon

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Mar 3, 2017

I'll review this.

djgrant and others added some commits Jan 23, 2017

@Timer Timer force-pushed the djgrant:enhancement/modularise-scripts branch from 4908921 to 38bc01c Mar 3, 2017

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Mar 3, 2017

Should be good, @gaearon -- I believe I captured all the missing changes in 38bc01c.

}

if (showInstructions) {
if (typeof onReadyCallback === 'function') {

This comment has been minimized.

Copy link
@gaearon

gaearon Mar 3, 2017

Member

It is confusing that we only call it when showInstructions is true.
We should instead pass it as an argument.

@gaearon

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

Looks good aside from one nit.

Timer added some commits Mar 4, 2017

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2017

Before:

Copying files into /Users/joe/Desktop/test2
  Adding config/env.js to the project
  Adding config/paths.js to the project
  Adding config/polyfills.js to the project
  Adding config/webpack.config.dev.js to the project
  Adding config/webpack.config.prod.js to the project
  Adding config/jest/cssTransform.js to the project
  Adding config/jest/fileTransform.js to the project
  Adding scripts/build.js to the project
  Adding scripts/start.js to the project
  Adding scripts/test.js to the project

After:

Copying files into /Users/joe/Desktop/test1
  Adding /config/env.js to the project
  Adding /config/paths.js to the project
  Adding /config/polyfills.js to the project
  Adding /config/webpack.config.dev.js to the project
  Adding /config/webpack.config.prod.js to the project
  Adding /config/webpackDevServer.config.js to the project
  Adding /config/jest/babelTransform.js to the project
  Adding /config/jest/cssTransform.js to the project
  Adding /config/jest/fileTransform.js to the project
  Adding /scripts/build.js to the project
  Adding /scripts/start.js to the project
  Adding /scripts/test.js to the project
  Adding /scripts/utils/addWebpackMiddleware.js to the project
  Adding /scripts/utils/createWebpackCompiler.js to the project

Looks like we accidentally included /config/jest/babelTransform.js. Removed in 61e99ee.

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2017

Thanks so much for this, @djgrant! Your efforts will help many people who maintain forks of create-react-app. 😄

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

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2017

AppVeyor CI green on my repo.

@Timer Timer merged commit b88d665 into facebook:master Mar 4, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gaearon gaearon referenced this pull request Mar 4, 2017

Closed

Modularise scripts #1431

@djgrant djgrant deleted the djgrant:enhancement/modularise-scripts branch Mar 4, 2017

@djgrant

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2017

Smashing, great to see this in master! Thanks @Timer and @gaearon for ironing it out!

path.join('scripts', 'test.js')
];
// Make shallow array of files paths
var files = folders.reduce(function (files, folder) {

This comment has been minimized.

Copy link
@tuchk4

tuchk4 Mar 6, 2017

Contributor

@Timer btw. Is there is any code convention (besides of eslint)? Use normal function or arrow?
Right now there are both normal and arrow are using. For example - eject.js.

This comment has been minimized.

Copy link
@Timer

Timer Mar 6, 2017

Collaborator

We used to have a lower node requirement than Node 4, iirc. I'm not sure on an official preference.

This comment has been minimized.

Copy link
@gaearon

gaearon Mar 6, 2017

Member

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

SpaceK33z added a commit to CodeYellowBV/create-react-cy-app that referenced this pull request Mar 7, 2017

Modularise scripts (facebook#1433)
* 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

kohver added a commit to stockspiking/create-react-app that referenced this pull request Mar 9, 2017

Modularise scripts (facebook#1433)
* 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

WiNloSt added a commit to prontotools/create-react-app that referenced this pull request Apr 4, 2017

Modularise scripts (facebook#1433)
* 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

sbuzonas pushed a commit to sbuzonas/react-scripts that referenced this pull request May 5, 2017

Modularise scripts (facebook#1433)
* 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

@lock lock bot locked and limited conversation to collaborators Jan 21, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.