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

Document forking react-scripts instead of ejecting #779

Closed
wants to merge 6 commits into from

Conversation

thien-do
Copy link
Contributor

@thien-do thien-do commented Sep 27, 2016

I follow @shubheksha instruction and rewrite it as a section in User Guide. I hope it can help.

Because I didn't see anyone claim to work on this yet, so I want to start a PR and we can discuss more. In case @zperrault or anyone have worked on this already, I am totally ok to close my PR.

To be honest, I know I don't have much experience writing docs but I am very active and have much time to update my PR to be better/more useful in case anyone have any suggestion.

Related issue: #682

UPDATE:
A summary of current things that need to do:

  1. Fork CRA
  2. Change react-scripts package name (in its package.json) (prefer scoped)
  3. Mark other packages as private
  4. Run npm run publish from root
  5. Use it with CRA

@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 cla@fb.com. Thanks!

@ghost ghost added the CLA Signed label Sep 27, 2016
@ghost
Copy link

ghost commented Sep 27, 2016

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

4: [Publish](https://docs.npmjs.com/getting-started/publishing-npm-packages) your customized `react-scripts` package to npm. Remember to cd into `/packages/react-scripts/` before you publish.
```sh
$ cd packages/react-scripts
$ npm publish

Choose a reason for hiding this comment

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

I tried to follow your instructions, but this will not publish a working react-scripts package. The package needs to be published with create-react-app's npm script npm run publish, as this will do some cleanup tasks before publishing react-scripts to npm. Check this out for more info: https://github.com/facebookincubator/create-react-app/blob/master/CONTRIBUTING.md#cutting-a-release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh thank you a lot. I didn't know that. I don't know why it works in my case.. I'll update the above step

Copy link
Contributor Author

@thien-do thien-do Sep 28, 2016

Choose a reason for hiding this comment

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

oh @Mattii I think it is because I commented out those "remove-on-publish" block with this suggestion #682 (comment)..

Copy link
Contributor Author

@thien-do thien-do Sep 28, 2016

Choose a reason for hiding this comment

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

I updated it! Thank you. However, I don't know should we say something about commenting those "remove-on-publish" block.. it won't affect the process, but it seems some people use npm link instead of publish the package.

I think maybe we should wait for this #765

Choose a reason for hiding this comment

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

Yes, commenting out these lines helped me to fix it in the first place :)
I didn't know about #765, but getting rid of this remove-on-publish hook seems to be the best idea, otherwise npm link only works with this commenting-hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know is there any difference between running npm publish in react-scripts vs npm run publish in create-react-apps, except to remove remove-on-publish block?

I met an issue with npm run publish: when I pull new update from upstream, and there are some updates in other packages than react-scripts (babel-preset-react-app for example), it ask me to publish them too, so I cannot publish my react-scripts. Is there a way to run npm run publish but skip other packages?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps --scope could help? Something like npm run publish -- --scope react-scripts. I have no idea if that works though.

cc @hzoo who might say

Copy link

@hzoo hzoo Sep 30, 2016

Choose a reason for hiding this comment

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

To my knowledge --scope is only run run/exec.

There's https://github.com/lerna/lerna#--force-publish-packages --force-publish for a specific package I believe (which could be renamed to be scope as a future thing).

The normal lerna publish runs a git diff so if anything changes it will try to update any packages that have changed.

There's also https://github.com/lerna/lerna#--only-explicit-updates --no-explicit-updates to only make a new version for the actual packages that have changed (takes adv of ^) but wouldn't make sense for a major version bump.

Hopefully that helps.

Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Great job. Left a few comments. We also need to figure out how to avoid publishing any other packages.

@@ -936,6 +937,50 @@ Install the Surge CLI if you haven't already by running `npm install -g surge`.

Note that in order to support routers that use html5 `pushState` API, you may want to rename the `index.html` in your build folder to `200.html` before deploying to Surge. This [ensures that every URL falls back to that file](https://surge.sh/help/adding-a-200-page-for-client-side-routing).

## Fork instead of eject
Copy link
Contributor

@gaearon gaearon Sep 30, 2016

Choose a reason for hiding this comment

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

Let’s call this Forking as Alternative to Ejecting, with capitalized words.

@@ -936,6 +937,50 @@ Install the Surge CLI if you haven't already by running `npm install -g surge`.

Note that in order to support routers that use html5 `pushState` API, you may want to rename the `index.html` in your build folder to `200.html` before deploying to Surge. This [ensures that every URL falls back to that file](https://surge.sh/help/adding-a-200-page-for-client-side-routing).

## Fork instead of eject

If you want to customize the default configuration slightly (e.g. add CSS Modules, SASS, decorators…), while still having all future updates and a one dependency, you can fork this repo and create your own configuration of `react-scripts` and use it with `create-react-app`.
Copy link
Contributor

Choose a reason for hiding this comment

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

We style it as "Create React App" throughout this guide instead of create-react-app.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to explain the tradeoffs here. What are the benefits and downsides of this approach?


If you want to customize the default configuration slightly (e.g. add CSS Modules, SASS, decorators…), while still having all future updates and a one dependency, you can fork this repo and create your own configuration of `react-scripts` and use it with `create-react-app`.

### Instruction
Copy link
Contributor

Choose a reason for hiding this comment

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

This subtitle can be removed


### Instruction

1: Fork [create-react-app repo](https://github.com/facebookincubator/create-react-app).
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 say "Fork the create-react-app repository"

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 use "1. " instead of "1: "

Copy link
Contributor Author

@thien-do thien-do Sep 30, 2016

Choose a reason for hiding this comment

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

I'm sorry but I don't know how to make it works with code block in numbered list. The item after the code block is reset to 1:

  1. Some text
some code
  1. Another text

Can you suggest a solution? I use "1:" because I don't know how to use "1."

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 remove the numbers altogether then

Copy link
Contributor Author

@thien-do thien-do Sep 30, 2016

Choose a reason for hiding this comment

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

Sure, I'll update in an hour


1: Fork [create-react-app repo](https://github.com/facebookincubator/create-react-app).

2: Change the name of `react-scripts` package to your new one, and also reset its version.
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 say "to the name you chose for your fork" instead of "to your new one".


2: Change the name of `react-scripts` package to your new one, and also reset its version.
```js
// /packages/react-scripts/package.json
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 replace this comment with a small header, just like in all examples above.


3: Make your changes inside `react-scripts` package. For example, add CSS Modules:
```js
// /packages/react-scripts/config/webpack.config.dev.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

```

4: Publish your customized `react-scripts` package with `create-react-app`'s npm script `npm run publish`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Publish your fork by running npm run publish in the root of the forked repository.

```
It is a long process. At the end, you will be asked to update the version.

5: Now you can use your customized setup with `create react app`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Now you can create React apps using your fork of react-scripts:

```

### Note
- [create-react-app](https://github.com/facebookincubator/create-react-app) is a [monorepo](https://github.com/babel/babel/blob/master/doc/design/monorepo.md) that contains `react-scripts` package. This is the heart of CRA, with all configurations and scripts. You only need to change and publish this package, not the whole repo.
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 not use CRA acronym here.

@thien-do
Copy link
Contributor Author

@gaearon thank you a lot. I updated the Readme as you suggested. However, there is 1 thing I can't, please see my comment (#779 (comment)) above

@thien-do
Copy link
Contributor Author

Hi @gaearon , I have updated the README to not using numbered list at all.

So far, we have 2 issues:

  1. We can't npm run publish from root because it will ask to publish other packages too
  2. We suggested to use scope package. However, I'm not sure how to specify --access public (because it seems by default scope package will be published as private). Or we can run npm access public after publishing? @ForbesLindesay

@eXtreme
Copy link

eXtreme commented Oct 3, 2016

@dvkndn
ad. 2 - I've tried scoped package with my fork, run npm publish --access public and then only npm publish and it works like a charm. It won't allow running npm publish without setting it as public if you don't have paid npm account.

@hzoo
Copy link

hzoo commented Oct 3, 2016

regarding 1. @dvkndn Did you see my comment? #779 (comment)

@thien-do
Copy link
Contributor Author

thien-do commented Oct 3, 2016

@eXtreme I myself use that solution (you also comment out those remove-on-publish block, right?), and I think many people also do that. However, it is currently suggested that we use "npm run publish" script from root #779 (comment)..

Anyway, it seems we should note that it is suggested to run "npm publish --access public" first?

@hzoo yes thank you I saw your comment.

--only-explicit-updates doesn't help us in this case because we want to skip publish a package that actually changed. For example, babel-preset-react-app have some changes from mainstream because we pulled the whole repo, but it shouldn't be published.

I'm trying --force-publish-packages.. It takes pretty long so I will update the result here later..

@thien-do
Copy link
Contributor Author

thien-do commented Oct 3, 2016

@hzoo it seems --force-publish doesn't work in our case too

...
+ ./node_modules/.bin/lerna publish --independent --force-publish=quoine-react-scripts
Lerna v2.0.0-beta.29
Independent Versioning Mode
Checking for updated packages...
? Select a new version for babel-preset-react-app (currently 0.2.1) (Use arrow keys)
❯ Patch (0.2.2)
  Minor (0.3.0)
  Major (1.0.0)
  Custom

cliedeman added a commit to stackworx/create-react-app that referenced this pull request Dec 4, 2016
@gaearon gaearon mentioned this pull request Dec 10, 2016
- You still have all Create React App features and updates

Cons:
- You need to maintain your fork, [make sure it is synced](https://help.github.com/articles/fork-a-repo/#keep-your-fork-synced) with the upstream to have all updates. [Backstroke](https://github.com/1egoman/backstroke) is a bot that can help you with this.
Copy link

Choose a reason for hiding this comment

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

It might be worth linking to this comment explaining that the "classic" webhook method should be used if one wants Backstroke to assist in keeping the fork synced.

Choose a reason for hiding this comment

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

As per backstrokeapp/server#52:
the "classic" web hook method should no longer be required.

I have backstroke setup using the standard method...
Will let you know how it goes when something is pushed to the upstream 📦

Copy link

Choose a reason for hiding this comment

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

I have been successfully merging commits with @1egoman's backstroke bot for the last 4 or 5 commits. It has been working beautifully.

@gaearon
Copy link
Contributor

gaearon commented Feb 11, 2017

Is there something that needs to be done to bring this up to date? Thanks!

@thien-do
Copy link
Contributor Author

@ervoiox I'm sorry it's been a long time :( I don't remember the exact commands now. But I can sure that I don't need to have a paid account. I will do again and will update with detailed information

@gaearon yes there are works that need to be done to update this (actually, the update instruction will be simpler than the original one, and less issues to be mentioned)

However, I can't find time until 1 March. So if anyone have time and want to take this I'd be very glad to help. If not, I will update this in March

@gaearon
Copy link
Contributor

gaearon commented Feb 24, 2017

Thanks, I think it's fine to land this in March. Advanced users can find it here as a work in progress anyway.

@gaearon
Copy link
Contributor

gaearon commented Mar 6, 2017

Just a heads up we released 0.9.4 with a few improvements for people who fork react-scripts: https://github.com/facebookincubator/create-react-app/releases/tag/v0.9.4. In particular, scoped react-scripts packages should work now, and ejecting should still work even if you rename the binary. Big thanks to @tuchk4 for pushing these changes through, and to @Timer, @viankakrisna, and @n3tr for help with fixing CI to work on Windows.

We care about experience of forkers, and if there's more things we should fix, please file issues. The only gotcha right now is that we're releasing from 0.9.x branch since Webpack integration in master still has issues. So we recommend releasing based on 0.9.x as well.

@thtliife
Copy link

thtliife commented Mar 9, 2017

@gaearon

We care about experience of forkers, and if there's more things we should fix, please file issues. The only gotcha right now is that we're releasing from 0.9.x branch since Webpack integration in master still has issues. So we recommend releasing based on 0.9.x as well.

Is there a way forkers are able to easily tell which branch you are releasing from, so as to keep our forks as close to the upstream releases as possible?
For instance, if/when you begin releasing from the master branch again, we know to follow suit?

@Timer
Copy link
Contributor

Timer commented Mar 9, 2017

The easiest way would be to look at https://github.com/facebookincubator/create-react-app/releases, which specifies what branch it was cut from.

vs

Maybe we could change the default branch on the repo to 0.9.x for now, @gaearon?

In the future we might want to keep master for the current cut releases, doing any for-future releases on a next branch. This would yield the best ergonomics for forkers.

Also, when we cut 0.10 we will be releasing from master again.

@thtliife
Copy link

thtliife commented Mar 9, 2017

The easiest way would be to look at https://github.com/facebookincubator/create-react-app/releases, which specifies what branch it was cut from.

Doh!...
github needs a face palm emoji...
I really need to think before I post... :P

@thien-do
Copy link
Contributor Author

thien-do commented Apr 3, 2017

Hi guys, I'm sorry but can someone take over this PR? I have no time recently nor in the near future to handle this :(

@Timer
Copy link
Contributor

Timer commented Apr 3, 2017

We'll get it taken care of. Thanks for letting us know!

@Timer Timer added this to the 0.10.0 milestone Apr 3, 2017
@Timer Timer mentioned this pull request May 5, 2017
32 tasks
@gaearon gaearon modified the milestones: 0.10.1, 0.10.0, 1.0.1, 1.0.x May 16, 2017
@boda-sh
Copy link

boda-sh commented Nov 27, 2017

how about using react-app-rewired? IMO it's a lot easier than forking react-scripts etc.

For example, I've been using react-app-rewired and one of its loaders react-app-rewire-css-modules to achieve CSS Modules in CRA without ejecting.

@Timer
Copy link
Contributor

Timer commented Nov 27, 2017

react-app-rewired can break at any time, so you shouldn't rely on it for a production application. Plus it sorta breaks the whole eject flow.

@gaearon
Copy link
Contributor

gaearon commented Jan 8, 2018

In the end I went with #3710 instead, but thanks for your effort!

@gaearon gaearon closed this Jan 8, 2018
@thien-do thien-do deleted the patch-1 branch January 11, 2018 08:51
@lock lock bot locked and limited conversation to collaborators Jan 20, 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.