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

dependencies: move chalk to react-dev-utils #6150

Merged
merged 1 commit into from
Jan 12, 2019

Conversation

evaporei
Copy link
Contributor

@evaporei evaporei commented Jan 8, 2019

This is being done to minimize the amount of dependencies
are expose to the end user when running a npm run eject.
Related to issue: #751.

I thought since chalk is probably not used directly by the end user, it could be on react-dev-utils instead of directly on react-scripts dependencies.

@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. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

This is an interesting idea.

It looks like you haven't actually removed chalk from the react-scripts package dependencies - it shouldn't be needed there with this PR.

I think it would be good to get some thoughts from @Timer or @iansu before going ahead with this.

@evaporei
Copy link
Contributor Author

evaporei commented Jan 9, 2019

Oops haha, it wasn't intentional. Wouldn't be misleading to have a package with it's dependencies listed on package.json and it actually gets from elsewhere?

@evaporei
Copy link
Contributor Author

evaporei commented Jan 9, 2019

Also @mrmckeb, I didn't understand how this would work 🤔
I mean, my objective with this PR is to stop showing chalk to a user that ejects it's app. That means that this dependency is still used by the create-react-app, however it will not show on the user's package.json.

By not removing the dependency from react-scripts, the dependency will be there, so the ideia for this PR will just not work.

Please explain to me if I understood wrongly, but as far as I've seen on https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/scripts/eject.js#L178, the code just copies the dependencies from react-scripts into the app created on eject. That is why, as far as I know, that dependencies listed on react-dev-utils are not listed to the end user.

@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 9, 2019

Hi @otaviopace, the issue is that you've now changed all usage of 'chalk' to user react-dev-utils. This is fine, but then why would react-scripts still need a reference to chalk when it has react-dev-utils as a dependency.

I understand why react-dev-utils would.

@evaporei evaporei force-pushed the move-chalk-to-react-dev-utils branch from 7f50005 to 9a7ce2f Compare January 9, 2019 10:53
@evaporei
Copy link
Contributor Author

evaporei commented Jan 9, 2019

Ohh, I see @mrmckeb , I've fixed the issue 🙂

@evaporei evaporei force-pushed the move-chalk-to-react-dev-utils branch from 9a7ce2f to 56a2b04 Compare January 9, 2019 12:52
@evaporei
Copy link
Contributor Author

evaporei commented Jan 9, 2019

Hmm, the ci failed 🤔
Do you know why?

@evaporei evaporei force-pushed the move-chalk-to-react-dev-utils branch from 56a2b04 to 25b20a3 Compare January 10, 2019 02:36
@evaporei
Copy link
Contributor Author

I've fixed one of the ci errors, however the last one is the same that is happening on the master branch.

Do you know what it is? Can I help to fix it, I don't understand why this happens 😕

screenshot-of-error

@ianschmitz
Copy link
Contributor

Hi @otaviopace. We're currently working on the CI builds to fix what you've noticed here. Sorry it's holding you up.

@evaporei
Copy link
Contributor Author

No problem @ianshmitz! I am not on a hurry 🙂

@evaporei evaporei force-pushed the move-chalk-to-react-dev-utils branch from 25b20a3 to 304491e Compare January 11, 2019 15:08
This is being done to minimize the amount of dependencies
are expose to the end user when running a `npm run eject`.
Related to issue: facebook#751
@evaporei
Copy link
Contributor Author

evaporei commented Jan 11, 2019

Yesss, I rebased my branch with the master branch and it fixed the errors on CI. Congrats @ianschmitz 🎉 🎊
Could you please review this Pull Request again?

@Timer
Copy link
Contributor

Timer commented Jan 11, 2019

Please sign the CLA and we can get this merged. 😄

@evaporei
Copy link
Contributor Author

@Timer, done! I've signed the CLA 🙂

@facebook-github-bot
Copy link

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

@mrmckeb mrmckeb merged commit 47e9e2c into facebook:master Jan 12, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 12, 2019

Thanks @otaviopace, great work :)

@evaporei evaporei deleted the move-chalk-to-react-dev-utils branch January 12, 2019 15:29
@evaporei
Copy link
Contributor Author

Yayyyy!!!! 🎊

@lock lock bot locked and limited conversation to collaborators Jan 17, 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.

6 participants