Commit after CRA project creation

[melanieseltzer]

Related Issue:

Fixes #285

Summary:

Since CRA 2.0 automatically does a git init after project creation, the package.json with the Guppy info gets picked up as an uncommitted change, preventing functionality like Eject from running.

Handling this is as simple as using exec to do a git add and git commit. The resulting git log for the project is:

commit 5664397208c9bda6110efd0f5755f6ccd54e4ea2 (HEAD -> master)
Author: Melanie Seltzer <melleh11@gmail.com>
Date:   Tue Oct 9 15:08:21 2018 -0700

    Add to Guppy

commit 2014d676bff55e6d895722cc425b4a18ce1f1eca
Author: Melanie Seltzer <melleh11@gmail.com>
Date:   Tue Oct 9 15:08:21 2018 -0700

    Initial commit from Create React App

I kept the commits separate but if we want to minimize the log, we could just do a git commit --amend --no-edit and lump the Guppy change in with CRA's initial commit. However I feel like we should probably keep things separate, to make sure they can tell when the Guppy change comes in... but I don't feel strongly either way.

Haven't added this check to importing functionality yet, because I'm not sure how to handle it since we need to check for CRA AND whether there's a git repo. Maybe read the directory and check for .git folder... ?

[AWolf81]

I think to keep this PR small it's OK to move Git commit at the beginning of eject saga handler.

We can add the app setting for auto committing & Git check later.

[joshwcomeau]

Ah, I missed @AWolf81's comment at the end of his review, my bad.

Hm, I think we should stay away from creating automatic commits before eject. Long-term I like the idea of Guppy managing git stuff, but for now, we can recommend that users install a Git desktop app.

It's also important to remember that ejecting should only be done by advanced users anyway, so I think it's OK to assume they have Git knowledge (or to prompt them to acquire some before continuing). I'd be more worried if it was for a different task.

[melanieseltzer]

Ahh interesting, I didn't know having a dirty working state in Git would block eject!

Yeah, we didn't run into it til CRA 2.0 launched 😄 Good thing it happened cause we weren't handling it before.

So just wanted to clarify going forward (since there's been a few conflicting ideas here). This is what I'm going with:

• Keep it to just commit the package.json after project creation, and don't do an auto commit before ejecting.
• Keep the git command explicit git add package.json && git commit -m "Add Guppy data to package.json"
• Wrap in a try/catch to swallow errors
• If user has a dirty repo when trying to eject at a later time, throw a dialog error message up letting them know, and then leave it to them to solve (rather than auto committing before ejecting). Also give them a link to a git intro if they want it, as one of the dialog options. This is a separate issue so will be in a separate PR.

Let me know if anything above seems off (or I've misread anything) with the above before I continue work :)

[codecov[bot]]

Codecov Report

Merging #286 into master will decrease coverage by 0.01%. The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
- Coverage   20.09%   20.07%   -0.02%     
==========================================
  Files         235      235              
  Lines        3648     3651       +3     
  Branches      368      369       +1     
==========================================
  Hits          733      733              
- Misses       2648     2650       +2     
- Partials      267      268       +1

Impacted Files	Coverage Δ
src/services/create-project.service.js	31.81% <0%> (-2.33%)	:arrow_down:

[superhawk610]

I'm not sure where this falls morally (😅) but we could simply amend the initial commit to include Guppy's changes to package.json, something like

git add package.json && git commit --amend --no-edit
                                             ^ since we don't want an interactive prompt

[melanieseltzer]

I actually initially considered doing an amend, but IMO I think it's better to keep them separate. So it's very clear when the Guppy change to package.json came in rather than it getting muddled up with CRA stuff.

[superhawk610]

Oh haha you literally suggested that in your post, totally missed that 🙃

Personally I'm all for automation, but I can also understand people not wanting things done to their codebase without their approval, so I like @AWolf81's suggestion of having it be toggleable on/off.