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

add command to add files to staging after eject #5960

Merged
merged 6 commits into from
Mar 12, 2019
Merged

add command to add files to staging after eject #5960

merged 6 commits into from
Mar 12, 2019

Conversation

clickclickonsal
Copy link
Contributor

@clickclickonsal clickclickonsal commented Dec 4, 2018

#5953 - This adds the scripts & config folder to the git staging area after the app is ejected.

image

@netlify
Copy link

netlify bot commented Dec 4, 2018

Deploy preview for create-react-app ready!

Built with commit 209e28f7d8ea28cee8a985e67224732ffa3d5f1f

https://deploy-preview-5960--create-react-app.netlify.com

@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!

@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!

@iansu
Copy link
Contributor

iansu commented Dec 4, 2018

It would be nice if we could add a basic test for this, maybe in tasks/e2e-simple.sh. In there we test the eject command so it should be relatively easy to just run a command to verify that there are staged files after ejecting. What do you think?

@clickclickonsal
Copy link
Contributor Author

I think thats a good suggestion Would something like this suffice?
if [ -z "$(git diff --staged --name-only)" ]; then exit 1; fi

@iansu
Copy link
Contributor

iansu commented Dec 4, 2018

Yes, a git command like that should do it. I'm not a bash expert but I think you can do something like test -z "$(git diff --staged --name-only)". That basically does exactly what your if is doing and I believe will exit the script if the test fails. We're doing something similar in the exists method in e2e-simple.

@Timer
Copy link
Contributor

Timer commented Dec 4, 2018

I imagine if this fails it's not a big deal? Maybe we can remove the log info and wrap it in a try catch that swallows the error?

Also, we need the git detection before trying this right?

@clickclickonsal
Copy link
Contributor Author

That's a good catch @Timer. Wrapping it in a try catch would serve as a git detection as well.
I went ahead and moved the git add into a separate function & only doing the logging if it succeeds;

}
);
} catch (e) {
return '';
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably just return instead of returning an empty string. What get's returned if this succeeds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about just returning but then I saw we're retuning an empty string on line #37 so I did to keep it consistent.

I forgot to return true so I'll add that in. 😓

Copy link
Contributor

Choose a reason for hiding this comment

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

That other function returns a string on success and an empty string on failure. If this function returns true on success then it should return false on failure.

@@ -310,6 +324,11 @@ inquirer
console.log(green('Ejected successfully!'));
console.log();

if (tryGitAdd(appPath)) {
console.log(cyan('Added ejected files to Staging for commit.'));
Copy link
Contributor

Choose a reason for hiding this comment

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

The message can probably just be: Staged ejected files for for commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

for for was a typo. Please remove one of them.

@iansu
Copy link
Contributor

iansu commented Dec 5, 2018

Please add the test that we discussed earlier.

@stale
Copy link

stale bot commented Jan 7, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jan 7, 2019
@iansu iansu closed this Jan 7, 2019
@iansu iansu reopened this Jan 7, 2019
@iansu iansu removed the stale label Jan 7, 2019
@clickclickonsal
Copy link
Contributor Author

Hey @iansu, not sure if you saw but I added the test we discussed. 😄

@stale
Copy link

stale bot commented Feb 7, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Feb 7, 2019
@iansu iansu removed the stale label Feb 7, 2019
@stale
Copy link

stale bot commented Mar 9, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Mar 9, 2019
@iansu iansu self-assigned this Mar 10, 2019
@iansu iansu added this to the 3.0 milestone Mar 11, 2019
@iansu iansu closed this Mar 11, 2019
@iansu iansu reopened this Mar 11, 2019
@iansu iansu merged commit dc133a3 into facebook:master Mar 12, 2019
@iansu
Copy link
Contributor

iansu commented Mar 12, 2019

Thanks!

@clickclickonsal clickclickonsal deleted the git-add-after-eject branch March 12, 2019 21:09
JoviDeCroock added a commit to JoviDeCroock/create-react-app that referenced this pull request Mar 15, 2019
* masterd: (24 commits)
  Add TypeScript linting support (facebook#6513)
  Support React Hooks (facebook#5602) (facebook#5997)
  Support browserslist in @babel/preset-env (facebook#6608)
  Add empty mock for http2 (facebook#5686)
  Add note about npx caching (facebook#6374)
  change named import into default import (facebook#6625)
  Stage files for commit after ejecting (facebook#5960)
  Upgrade dependencies (facebook#6614)
  Make compiler variable const instead of let (facebook#6621)
  Type check JSON files (facebook#6615)
  Change class components to functional components in templates (facebook#6451)
  Convert JSON.stringify \n to os.EOL when writing tsconfig.json (facebook#6610)
  Update html-webpack-plugin (facebook#6361)
  Enable click to go to error in console for TypeScript (facebook#6502)
  Update webpack-dev-server to 3.2.1 (facebook#6483)
  [docs] revert removal of newlines from html (facebook#6386)
  Publish
  Prepare 2.1.8 release
  Reapply "Speed up TypeScript v2 (facebook#6406)" (facebook#6586)
  Publish
  ...

# Conflicts:
#	packages/babel-preset-react-app/create.js
#	packages/react-scripts/scripts/build.js
@lock lock bot locked and limited conversation to collaborators Mar 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.

4 participants