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

Create git repository with initial commit #1288

Merged
merged 11 commits into from
Jan 19, 2018
Merged

Conversation

mauricedb
Copy link
Contributor

Fixes #1244

The intended test plan was:

  1. Create a new React application on a machine with no Git installed. The application should be created with no additional artifacts just like before.
  2. Create a new React application in a folder that is not part of a git repository. The application should be created with an additional new git repository. The initial application source files should be committed to the git repository as the initial commit.
  3. Create a new React application in a folder that is already part of an existing git repository. The application should be created with no additional artifacts just like before.

However because I am running Windows natively I was unable to test this the way I wanted because the cra.sh doesn't run. Trying to do so in a Docker based Linux container was also less than successful.

@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 [email protected]. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions.

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

@mauricedb
Copy link
Contributor Author

Added the docs to the readme.

@mauricedb
Copy link
Contributor Author

@gaearon, Just wondering about this PR. It's been done for quite a while now. Thanks

gaearon
gaearon previously requested changes Feb 8, 2017

execSync('git init', {stdio: 'ignore'});
execSync('git add .', {stdio: 'ignore'});
execSync('git commit -m "chore: initial commit from create-react-app"', {stdio: 'ignore'});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just make this Initial commit? I don't think a lot of people are familiar with what "chore" means in this context, and many don't use this format.

try {
execSync('git --version', {stdio: 'ignore'});

if (insideGitRepository()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also bail if we're inside a Mercurial repository.

@gaearon
Copy link
Contributor

gaearon commented Feb 8, 2017

Sorry I haven't been very responsive. Lots of work on React 😛
I left a few notes.

@mauricedb
Copy link
Contributor Author

@gaearon, No problem, keep up the good work. Just making sure it doesn't get lost.

Made both the requested changes and rebased my commits on master.

@@ -1009,6 +1010,12 @@ Learn more about React Storybook:

You can turn your React app into a [Progressive Web App](https://developers.google.com/web/progressive-web-apps/) by following the steps in [this repository](https://github.com/jeffposnick/create-react-pwa).

## Git Repository
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest I think it's fine if don't specifically document this. I appreciate that you took the time to write it down but I don't think it's strictly necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaearon Not quite sure what you want me to do. Do you want me to leave it as it is there already or delete it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea let's just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, give me a minute.

@@ -64,6 +101,10 @@ module.exports = function(appPath, appName, verbose, originalDirectory, template
}
});

if (gitInit()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we init git here, doesn't this mean adding react and react-dom dependencies are not added yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch and a rather stupid mistake of me. Moved it to execute after adding react an and react-dom.

@mauricedb
Copy link
Contributor Author

mauricedb commented Feb 24, 2017

@gaearon Just checking if there any more changes you would like to see on this?

I see there are some conflicts now. Will rebase and resolve those later today.

Thanks

@Timer
Copy link
Contributor

Timer commented Feb 24, 2017

I think there was a bad merge, there's some unnecessary diffs that revert code from master.

@gaearon gaearon added this to the 0.10.0 milestone Feb 24, 2017
@mauricedb mauricedb force-pushed the git-init branch 5 times, most recently from 7447e25 to 21f0c51 Compare February 26, 2017 11:58
@mauricedb
Copy link
Contributor Author

@Timer I redid and fixed the rebase issues. I guess when Git said there where merge issues it really meant it this time :-(. Took a bit of time to figure out what went wrong but now I am only seeing my changes in the PR so the PR should be good.

However I am seeing the two TEST_SUITE=kitchensink tests fail on Travis when cleaning up. It is when removing the test-kitchensink directory. Not seeing why this is happening.

@gaearon gaearon added this to the 0.11.0 milestone May 16, 2017
@gaearon gaearon removed this from the 0.10.0 milestone May 16, 2017
@gaearon
Copy link
Contributor

gaearon commented May 16, 2017

Appreciate your work.
We're a bit short on time so I'm pushing this back to 0.11, but we'll look into it again at some point!

@mauricedb
Copy link
Contributor Author

mauricedb commented May 16, 2017 via email

@gaearon
Copy link
Contributor

gaearon commented Jan 15, 2018

We'll need to come back to this before cutting 2.0, in part because I now see this in a newly created project:

screen shot 2018-01-15 at 6 17 28 pm

@gaearon gaearon changed the base branch from master to next January 15, 2018 18:18
@mauricedb mauricedb force-pushed the git-init branch 2 times, most recently from ff91fbf to a3aea49 Compare January 16, 2018 10:17
@mauricedb mauricedb force-pushed the git-init branch 2 times, most recently from b7aaa1e to ff65875 Compare January 16, 2018 11:01
@mauricedb
Copy link
Contributor Author

@gaearon, Added two git commit statements to the tests that failed because of uncommitted changes. Still seeing the tests fail with a TypeError: Cannot read property 'loose' of undefined error that seems to be caused by babel-plugin-transform-destructuring not checking if options is an object.

@gaearon
Copy link
Contributor

gaearon commented Jan 16, 2018

Sorry, our CI had a bad day.

@mauricedb
Copy link
Contributor Author

@gaearon Seems TravisCI was having a bad CI day as well. If you restart the CI there, I don't have permissions to do so myself, it should turn green just like on AppVeyor.

@@ -134,6 +171,10 @@ module.exports = function(
}
}

if (gitInit()) {
console.log('Initializing git repository');

Choose a reason for hiding this comment

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

this is done after git initialized so i guess it's better to say git repository initialized rather than initializing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -104,7 +141,7 @@ module.exports = function(
args = ['install', '--save', verbose && '--verbose'].filter(e => e);
}
args.push('react', 'react-dom');

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can remove the space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

}

execSync('git init', {stdio: 'ignore'});
execSync('git add .', {stdio: 'ignore'});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be git add -A?
(infact git add . covers create and update but this one feels more inclusive)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sendilkumarn Sure, no problem but there really is no difference. Using git add -A is the same as git add . with Git 2. With Git 1 it isn't the same but the difference is in deleted files. As this is a newly created repo there is not going to be any deleted files.

@gaearon gaearon merged commit 247e5c9 into facebook:next Jan 19, 2018
@gaearon
Copy link
Contributor

gaearon commented Jan 19, 2018

Thanks!

@mauricedb
Copy link
Contributor Author

👍 Happy we finally got this merged in. Hope I can do more in the future.

akstuhl pushed a commit to akstuhl/create-react-app that referenced this pull request Mar 15, 2018
* Create git repo with initial commit

* Fixe commit message

* Added the git repo to the docs

* Bail if we are in a mercurial repository

* Removed Chore from commit mesage

* Create repo after installing react and react-dom

* Removed docs

* Commit changes when ejecting

* Update after review

* git add -A instead of git add . after code review
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
* Create git repo with initial commit

* Fixe commit message

* Added the git repo to the docs

* Bail if we are in a mercurial repository

* Removed Chore from commit mesage

* Create repo after installing react and react-dom

* Removed docs

* Commit changes when ejecting

* Update after review

* git add -A instead of git add . after code review
@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.

6 participants