Skip to content
This repository has been archived by the owner on Mar 9, 2021. It is now read-only.

upgraded default react-native version #228

Merged
merged 38 commits into from
Nov 30, 2018

Conversation

lvlrSajjad
Copy link
Contributor

@lvlrSajjad lvlrSajjad commented Nov 21, 2018

upgraded default react-native version to 0.57.7

updated react-navigation to 3.0.0

also updated ignite-dev-screens,redux,redux-persist and some other libs to the last version

after changes i got it running in android and it was ok

removed allowFontScaling from boilerplate/App/Config/index.js cause it causes error in new react-native version

@morgandonze
Copy link
Contributor

Thanks for the PR @lvlrSajjad.

Semaphoreci has some complaints:

standard: Use JavaScript Standard Style (https://standardjs.com)
standard: Run `standard --fix` to automatically fix some problems.
  /home/runner/ignite-ir-boilerplate-andross/boilerplate.js:181:30: Strings must use singlequote.
  /home/runner/ignite-ir-boilerplate-andross/boilerplate.js:181:61: Strings must use singlequote.
  /home/runner/ignite-ir-boilerplate-andross/boilerplate.js:183:5: Block must not be padded by blank lines.

Could you fix those please?

@lvlrSajjad
Copy link
Contributor Author

lvlrSajjad commented Nov 21, 2018

Thanks for the PR @lvlrSajjad.

Semaphoreci has some complaints:

standard: Use JavaScript Standard Style (https://standardjs.com)
standard: Run `standard --fix` to automatically fix some problems.
  /home/runner/ignite-ir-boilerplate-andross/boilerplate.js:181:30: Strings must use singlequote.
  /home/runner/ignite-ir-boilerplate-andross/boilerplate.js:181:61: Strings must use singlequote.
  /home/runner/ignite-ir-boilerplate-andross/boilerplate.js:183:5: Block must not be padded by blank lines.

Could you fix those please?

yes taken care of but it says

The `ignite generate` command must be run in an ignite-compatible directory

while it generates correctly
it seems even in the main version of ignite-andross-boilerplate we have same error i've tested a clone and got same errors
image

@lvlrSajjad
Copy link
Contributor Author

lvlrSajjad commented Nov 22, 2018

in semaphore log it seems we getting errors like these

Yarn supports the following semver range: "^4.8.0 || ^5.7.0 || ^6.2.2 || >=8.0.0"\nerror react-native@0.57.5: The engine "node" is incompatible with this module. Expected version ">=8.3".\nerror Found incompatible module\n{ Error: Command failed: yarn add react-native@0.57.5
Command `yarn add react-native@0.57.5 --exact` failed

i think the semaphor's yarn/npm version is in incompatible with react-native@0.57.5.

in my system i can run this test at least the first one successfully passes .

image

by its installation guid
https://reactnavigation.org/docs/en/getting-started.html
we need to add something to MyActivity class in android , not needed in ios.
so i patched file in boilerplate.js
@lvlrSajjad
Copy link
Contributor Author

lvlrSajjad commented Nov 26, 2018

Just ignite-dev-screens may not work with this version of react-navigation (3.0.0) so if you merge this , i will upgrade the plugin too
also i have semaphor's yarn version errors yet
more info about errors
Here
and
Here

@morgandonze
Copy link
Contributor

@lvlrSajjad we're removing Semaphore from this project and adding Circle CI instead. We can ignore the Semaphore failure and see whether it passes on Circle once it's ready.

@lvlrSajjad lvlrSajjad closed this Nov 28, 2018
@lvlrSajjad
Copy link
Contributor Author

@lvlrSajjad we're removing Semaphore from this project and adding Circle CI instead. We can ignore the Semaphore failure and see whether it passes on Circle once it's ready.

i just accidently closed the pull
@mlaco i must fetch your circle config to being testable with circle ?

@lvlrSajjad lvlrSajjad reopened this Nov 28, 2018
@morgandonze
Copy link
Contributor

@lvlrSajjad yes, you'll need the circle config, but it looks like you got it already.
We need to trigger Circle to test your branch. Please make an empty commit using
git commit --allow-empty -m "fix(ci): Testing CI release" to do so.

@lvlrSajjad
Copy link
Contributor Author

well it seems it can't find lint command in package.json of installed project . maybe it can't run ignite add standard

@morgandonze
Copy link
Contributor

well it seems it can't find lint command in package.json of installed project . maybe it can't run ignite add standard

Yep. I'm currently trying to figure out why. It's able to do it on the master branch, so I'm kind of stumped.

@lvlrSajjad
Copy link
Contributor Author

lvlrSajjad commented Nov 30, 2018

well it seems it can't find lint command in package.json of installed project . maybe it can't run ignite add standard

Yep. I'm currently trying to figure out why. It's able to do it on the master branch, so I'm kind of stumped.

i found problem it's something wrong with my react-navigation linking , i will cover it :-D @mlaco

@morgandonze
Copy link
Contributor

Congrats, it worked!

@morgandonze
Copy link
Contributor

morgandonze commented Nov 30, 2018

Let me just look over the code again before I merge (I want to get a second review too)

@lvlrSajjad
Copy link
Contributor Author

@mlaco Test passed but i need to find the problem with patch in file :-D cause i need to do that modifications on mainactivity.java as this link says
https://reactnavigation.org/docs/en/getting-started.html

@morgandonze morgandonze self-requested a review November 30, 2018 19:44
Copy link
Contributor

@morgandonze morgandonze left a comment

Choose a reason for hiding this comment

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

I asked our team for a second review, too

boilerplate.js Outdated
import com.facebook.react.ReactRootView;
import com.swmansion.gesturehandler.react.RNGestureHandlerEnabledRootView;`
})
//
Copy link
Member

Choose a reason for hiding this comment

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

Should this commented out section be left in without an explanation of why it's commented out in favor of the code block above 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.

for now i'm just commenting / uncommenting stuff till i findout what is wrong i'll say when i'm done

@@ -197,7 +219,7 @@ async function install (context) {
}

if (answers['redux-persist'] === 'Yes') {
await system.spawn(`ignite add redux-persist@"~>1.0.1" ${debugFlag}`, {
await system.spawn(`ignite add redux-persist@"~>5.10.0" ${debugFlag}`, {
Copy link
Member

Choose a reason for hiding this comment

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

HOLY CRAP that's a massive version upgrade!

@lvlrSajjad
Copy link
Contributor Author

Guys I'm done
@mlaco thank's for being patient and helping me to figure this out
@markrickert thank's for reviews

@morgandonze
Copy link
Contributor

@lvlrSajjad yes, thank you! I appreciate your hard work and dedication.

@morgandonze morgandonze merged commit 575d24c into infinitered:master Nov 30, 2018
infinitered-circleci pushed a commit that referenced this pull request Dec 1, 2018
# [2.4.0](v2.3.1...v2.4.0) (2018-12-01)

### Bug Fixes

* **ci:** Testing CI release ([ef979cc](ef979cc))
* **ci:** Testing CI release ([83e6a3d](83e6a3d))
* **ci:** Testing CI release ([4ff0748](4ff0748))

### Features

* **react-native:** upgraded default react-native version ([#228](#228)) ([575d24c](575d24c))
@infinitered-circleci
Copy link

🎉 This PR is included in version 2.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants