-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Rewrite 'react-native init' and 'react-native upgrade' without using Yeoman in preparation for templates support #10786
Conversation
LGTM so far |
Looking pretty good. I notice you forked the template/generated files. Please keep an eye on any changes to the old |
1b7fd7d
to
67dcefe
Compare
@ericvicenti Thanks for looking! Totally agree. Just updated the template to the latest version. git recognizes the files have moved so we can see the changes in them. |
* LICENSE file in the root directory of this source tree. An additional grant | ||
* of patent rights can be found in the PATENTS file in the same directory. | ||
* | ||
* @flow |
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.
is it flow?
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.
It's not good catch!
// Don't upgrade these files | ||
const fileName = path.basename(absoluteSrcFilePath); | ||
// TODO __tests__/index.ios.js | ||
// TODO __tests__/index.android.js |
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.
TODO relevant?
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.
Will fix this, thanks for the reminder!
: [argsOrName].concat(process.argv.slice(4)); // 'AwesomeApp' | ||
|
||
// args is ['AwesomeApp', '--verbose'] | ||
if (!args || args.lentgh == 0) { |
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 assume there is no verbose here?
I think it is not needed, slows down npm even more
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.
The args is for example ['AwesomeApp', '--verbose'].
Do you mean supporting --verbose
is not needed? It was added by the community to debug when 'npm install' helps, I'd keep it.
const jestDeps = `jest babel-jest jest-react-native babel-preset-react-native react-test-renderer@${reactVersion}`; | ||
if (yarnVersion) { | ||
console.log('Adding Jest...'); | ||
execSync(`yarn add ${jestDeps} --dev --exact`); |
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.
Martin, while you are at it, how about we provide a backdoor for CI systems and offline installs.
Recently Legocastle was not able to block a bug because we can't reproduce install offline.
Any ideas?
From the top of my head we could provide an override CLI command of some sort here.
And compose this command on CI to fake yarn install
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.
Interesting, let's talk about so I understand how it should work.
I think this PR is getting very large - can it be done as a separate commit?
@@ -23,6 +23,7 @@ | |||
"^[./a-zA-Z0-9$_-]+\\.png$": "RelativeImageStub" | |||
}, | |||
"testPathIgnorePatterns": [ | |||
"local-cli/templates/", | |||
"/node_modules/", |
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.
Is there anymore yeoman left?
Can it be removed from 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.
Plan remove it in a separate celebratory commit :)
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.
Prompt usage
'latest version?\nIf you ever made any changes ' + | ||
'to this file, you\'ll probably want to keep it.\n' + | ||
'You can see the new version here: ' + absoluteSrcFilePath + '\n' + | ||
'Please answer "keep" / "replace" for ' + relativeDestPath + ':'); |
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.
Maybe we could have a more "yeoman-ish" prompt with the status clearly displayed in colors and a Yes/No answer (default Yes).
The log about the changes and the HelloWorld app location should be displayed once and for all at the beginning.
Typing "keep" or "replace" for each file (typically 5 to 10 times) is boring.
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.
Good point that typing "keep" or "replace" is boring. Can make it "do you want to overwrite the file? y/n"?
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 prefer to default to 'n' - overwriting the files throws away the changes people made in them.
'You can see the new version here: ' + absoluteSrcFilePath + '\n' + | ||
'Please answer "keep" / "replace" for ' + relativeDestPath + ':'); | ||
const answer = prompt(); | ||
if (answer == 'replace') { |
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.
Double equal 😄
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.
If the user makes a typo in "replace", the file is kept. It could lead to a lot of madness :p
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.
Good catch about ==
, thanks!
If people make a mistake and keep files they wanted to overwrite, they can run 'react-native upgrade' again to start over.
} else if (contentChanged === 'identical') { | ||
return 'keep'; | ||
} else { | ||
throw new Error(`Unkown file changed state: {relativeDestPath}, {contentChanged}`); |
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.
Incorrect template string placeholders: missing $
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.
Awesome, thank you!
3e4fe71
to
f2adec5
Compare
TODO e2e test fails with:
iOS e2e test:
node node_modules/react-native/local-cli/cli.js start "--non-persistent" |
it was updated just a day ago On 16 November 2016 at 00:48, Martin Konicek [email protected]
|
59272d9
to
cf571ff
Compare
Flow check should now be fixed, Dan Caspi landed a fix for the iOS compile error (3b4ac79, rebased on top of it now). |
93b68fa
to
63d8964
Compare
63d8964
to
5053402
Compare
@facebook-github-bot shipit |
Sorry, I can't do that @hramos - this feature is only supported for non-employees; you can land via the internal pull request dashboard. |
Something went wrong when importing this pull request. Please cc someone from the team at fb to help with importing this. |
…Yeoman in preparation for templates support Summary: This is the manually imported version of #10786 This was mostly straigthforward by replacing the local-cli folder with the version I had in my local git checkout, plus a few other files I listed with git diff --name-only. Reviewed By: hramos Differential Revision: D4201118 fbshipit-source-id: 4d0fb54b0edda9de1abba427958e420fd2ac105c
Closed in a477aec. |
Summary: We stopped using Yeoman in #10786 I almost forgot to remove the now-unused dependency :) **Test Plan** - Published react-native to Sinopia - Ran `react-native init MyApp` - The app was generated correctly - The app's node_modules folder doesn't contain Yeoman Reviewed By: cpojer Differential Revision: D4291619 fbshipit-source-id: 44c1ef8035fa2d8c40d4e8c505207245e1a95d3c
Summary: We stopped using Yeoman in #10786 I almost forgot to remove the now-unused dependency :) **Test Plan** - Published react-native to Sinopia - Ran `react-native init MyApp` - The app was generated correctly - The app's node_modules folder doesn't contain Yeoman Reviewed By: cpojer Differential Revision: D4291619 fbshipit-source-id: 44c1ef8035fa2d8c40d4e8c505207245e1a95d3c
Summary: We stopped using Yeoman in #10786 I almost forgot to remove the now-unused dependency :) **Test Plan** - Published react-native to Sinopia - Ran `react-native init MyApp` - The app was generated correctly - The app's node_modules folder doesn't contain Yeoman Reviewed By: cpojer Differential Revision: D4291619 fbshipit-source-id: 44c1ef8035fa2d8c40d4e8c505207245e1a95d3c
Summary: We stopped using Yeoman in #10786 I almost forgot to remove the now-unused dependency :) **Test Plan** - Published react-native to Sinopia - Ran `react-native init MyApp` - The app was generated correctly - The app's node_modules folder doesn't contain Yeoman Reviewed By: cpojer Differential Revision: D4291619 fbshipit-source-id: 44c1ef8035fa2d8c40d4e8c505207245e1a95d3c
…Yeoman in preparation for templates support Summary: This is the manually imported version of facebook#10786 This was mostly straigthforward by replacing the local-cli folder with the version I had in my local git checkout, plus a few other files I listed with git diff --name-only. Reviewed By: hramos Differential Revision: D4201118 fbshipit-source-id: 4d0fb54b0edda9de1abba427958e420fd2ac105c
Summary: We stopped using Yeoman in facebook#10786 I almost forgot to remove the now-unused dependency :) **Test Plan** - Published react-native to Sinopia - Ran `react-native init MyApp` - The app was generated correctly - The app's node_modules folder doesn't contain Yeoman Reviewed By: cpojer Differential Revision: D4291619 fbshipit-source-id: 44c1ef8035fa2d8c40d4e8c505207245e1a95d3c
…Yeoman in preparation for templates support Summary: This is the manually imported version of facebook/react-native#10786 This was mostly straigthforward by replacing the local-cli folder with the version I had in my local git checkout, plus a few other files I listed with git diff --name-only. Reviewed By: hramos Differential Revision: D4201118 fbshipit-source-id: 4d0fb54b0edda9de1abba427958e420fd2ac105c
…Yeoman in preparation for templates support Summary: This is the manually imported version of facebook/react-native#10786 This was mostly straigthforward by replacing the local-cli folder with the version I had in my local git checkout, plus a few other files I listed with git diff --name-only. Reviewed By: hramos Differential Revision: D4201118 fbshipit-source-id: 4d0fb54b0edda9de1abba427958e420fd2ac105c
We'll be adding support for templates into 'react-native init', so you can create an app with tabbed navigation, Android drawer etc. to get started quickly.
In preparation for adding this feature, let's stop using Yeoman which we've wanted to do for quite a while - without Yeoman the code is shorter, easier to understand and we'll have less dependencies.
From user's perspective, this PR doesn't change the init experience - just replaces Yeoman with simpler code - see
copyProjectTemplateAndReplace
. The upgrade experience stays almost the same - we still ask how to resolve conflicts. The only thing missing is showing the difference between files but we'll replace that soon with https://github.com/ncuillery/rn-diffTest plan
npm publish
to Sinopia as described here: https://github.com/facebook/react-native/blob/master/react-native-cli/README.mdreact-native init AwesomeApp
creates an app, everything is named correctly, Java package iscom.awesomeapp
:react-native run-ios
andreact-native run-android
- the app starts, Reload JS works (this is also tested by Circle CI and Travis):Enabled HMR, edited and saved index.android.js, JS was hot-reloaded:
react-native upgrade
, answered 'n' to the question to replace:react-native upgrade
, answered 'y' to the question to replace: