-
-
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
Beaufity output of eject.js script #769
Conversation
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! |
I would prefer we keep the existing palette. We mostly use |
@@ -78,15 +81,15 @@ prompt( | |||
var appPackage = require(path.join(appPath, 'package.json')); | |||
|
|||
var ownPackageName = ownPackage.name; | |||
console.log('Removing dependency: ' + ownPackageName); | |||
console.log('Removing dependency: ' + yellowUnderline(ownPackageName)); |
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.
I think this should have the same indentation as “adding a dependency” but they both need to be grouped under “Updating package.json
“
delete appPackage.devDependencies[ownPackageName]; | ||
|
||
Object.keys(ownPackage.dependencies).forEach(function (key) { | ||
// For some reason optionalDependencies end up in dependencies after install | ||
if (ownPackage.optionalDependencies[key]) { | ||
return; | ||
} | ||
console.log('Adding dependency: ' + key); | ||
console.log(green('\tAdding dependency: ') + yellowUnderline(key)); |
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.
Let’s use two-space indentation like in other commands.
@@ -62,7 +65,7 @@ prompt( | |||
fs.mkdirSync(path.join(appPath, 'scripts')); | |||
|
|||
files.forEach(function(file) { | |||
console.log('Copying ' + file + ' to ' + appPath); | |||
console.log(green('Copying ') + yellowUnderline(file) + ' to ' + yellowUnderline(appPath)); |
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.
Let’s indent these and group them under “Copying files...”
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
@@ -76,21 +81,21 @@ prompt( | |||
|
|||
var ownPackage = require(path.join(ownPath, 'package.json')); | |||
var appPackage = require(path.join(appPath, 'package.json')); | |||
|
|||
console.log(cyan('Managing dependencies')); |
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.
Let’s call this Updating dependencies...
@@ -61,8 +64,10 @@ prompt( | |||
fs.mkdirSync(path.join(appPath, 'config', 'jest')); | |||
fs.mkdirSync(path.join(appPath, 'scripts')); | |||
|
|||
console.log(); | |||
console.log(cyan('Copying files')); |
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.
Let’s add ...
at the end of every group title
|
||
console.log('Updating scripts'); | ||
console.log(); | ||
console.log(cyan('Updating scripts')); |
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.
Same here, let’s add ...
@@ -107,20 +112,21 @@ prompt( | |||
true | |||
); | |||
|
|||
console.log('Writing package.json'); | |||
console.log(); | |||
console.log(cyan('Writing ') + 'package.json'); |
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.
And here
var ownPackageName = ownPackage.name; | ||
console.log('Removing dependency: ' + ownPackageName); | ||
console.log(red(' Removing dependency: ') + ownPackageName); |
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.
Let’s use yellow
here so it doesn’t feel like an error
|
||
console.log('Updating scripts'); | ||
console.log(); | ||
console.log(cyan('Updating scripts...')); | ||
delete appPackage.scripts['eject']; | ||
Object.keys(appPackage.scripts).forEach(function (key) { | ||
appPackage.scripts[key] = appPackage.scripts[key] |
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.
Let’s print each step here?
For example:
Replacing "react-scripts start" with "node scripts/start.js"
files.forEach(function(file) { | ||
console.log('Copying ' + file + ' to ' + appPath); | ||
console.log(cyan(' Copying ') + file+ ' to ' + appPath); |
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.
Can you try making file
and appPath
cyan instead of Copying
?
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.
Looking at the output, it prints the same giant appPath on every single line.
Suggestion: could we put this path in the header instead?
Copying files to /Users/.../tempApp ...
Copying .babelrc
Copying .eslintrc
delete appPackage.devDependencies[ownPackageName]; | ||
|
||
Object.keys(ownPackage.dependencies).forEach(function (key) { | ||
// For some reason optionalDependencies end up in dependencies after install | ||
if (ownPackage.optionalDependencies[key]) { | ||
return; | ||
} | ||
console.log('Adding dependency: ' + key); | ||
console.log(cyan(' Adding dependency: ') + key); |
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.
Similar, let’s make dependency name cyan
(or yellow
) instead of the label.
Sorry for the back-and-forth. Visual work takes some iterations. Can you try my suggestions? |
Can we show Jest, Babel and ESLint output too? |
Done. Added. |
Thank you @azakordonets :) |
delete appPackage.scripts['eject']; | ||
Object.keys(appPackage.scripts).forEach(function (key) { | ||
appPackage.scripts[key] = appPackage.scripts[key] | ||
.replace( | ||
new RegExp(ownPackageName + ' (\\w+)', 'g'), | ||
'node scripts/$1.js' | ||
); | ||
console.log(' Replacing react-scripts ' + cyan(key) + ' with ' + cyan(appPackage.scripts[key])); |
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.
Please make it so react-scripts
is also cyan here. We're replacing a complete command.
Also let's put both "react-scripts start/test/build"
and "node scripts/whatever.js"
in quotes.
Replacing "react-scripts start" with "node scripts/start.js"
This looks good to me. I left a comment about the last change I want to make. Thank you for working on this! |
To resolve conflicts: git remote add facebookincubator https://github.com/facebookincubator/create-react-app.git
git fetch facebookincubator
git merge facebookincubator/master You'll see the conflicts. After you resolve them, push the branch again. |
I like everything except the very last section.
I also think we can completely omit |
Would you like to have it all in cyan color, or should i only use cyan for "Adding" word ? |
All cyan is fine. |
This looks good to me! |
The only nit would be that |
Also quotes in |
This looks awesome. Thanks for sticking with it. |
Sweeet :) My pleasure . I'm very new to Javascript and React, so it's really great that i managed to contribute to this cool project. I checked other issues with "Help wanted" label, but they don't seem easy for me to pick up for now, but i'll continue tracking this project. Thanks for oportunity |
…react-app # By Dan Abramov (5) and others # Via Dan Abramov * 'master' of https://github.com/facebookincubator/create-react-app: docs(readme): peer dependencies applied (facebook#818) Fix typos on ISSUE_TEMPLATE.md (facebook#817) Add explicit linebreaks (facebook#813) Fix typo (facebook#810) Fix some typos (facebook#809) Beaufity output of eject.js script (facebook#769) Define process.env as object (facebook#807) Typo fix in webpack.config.dev.js comments (facebook#777) Add Netlify to deploy instructions Fix usage example to match react-dev-utils@0.2.x API Relaxed eslint rule no-unused-expressions (facebook#724) Fix the doc Publish Add 0.6.1 changelog Moved Babel and ESLint config to package.json after ejecting (facebook#773) Conflicts: packages/react-scripts/package.json
* Beaufity output of eject.js script * change formatting of the eject.js output and move colors to cyan * change message about file copy * add missing three dots to some statements in eject.js script * change color of "copying files" line and do not repeat copy path anymore in log * fix merge conflict * Remove yellow color from "Removing dependency" line * changing color to "Adding dependency" line * Add line that outputs which react script is getting replaced by similar node script * remove not used anymore colors * add console line about updating Jest configs * fix typo * change formatting of replacing script output in eject.js * remove "Writing package.json" file console output * make quotes cyan in "Replacing script" console output * update console log output for Jest, Babel, ESLint update and group them under one statement * Style nits
…react-app # By Dan Abramov (5) and others # Via Dan Abramov * 'master' of https://github.com/facebookincubator/create-react-app: docs(readme): peer dependencies applied (facebook#818) Fix typos on ISSUE_TEMPLATE.md (facebook#817) Add explicit linebreaks (facebook#813) Fix typo (facebook#810) Fix some typos (facebook#809) Beaufity output of eject.js script (facebook#769) Define process.env as object (facebook#807) Typo fix in webpack.config.dev.js comments (facebook#777) Add Netlify to deploy instructions Fix usage example to match react-dev-utils@0.2.x API Relaxed eslint rule no-unused-expressions (facebook#724) Fix the doc Publish Add 0.6.1 changelog Moved Babel and ESLint config to package.json after ejecting (facebook#773) Conflicts: packages/react-scripts/package.json
Solution for issue ##750
Since in
build.js
we already usechalk
library, i decided to reuse it in eject.js file. Operations are marked with green, file names and path's are marked with yellow and are underlined. Screenshots are attached .