Skip to content

RN61 upgrade#301

Merged
ethanshar merged 14 commits into
masterfrom
RN61-upgrade
Nov 26, 2019
Merged

RN61 upgrade#301
ethanshar merged 14 commits into
masterfrom
RN61-upgrade

Conversation

@ArnonZ

@ArnonZ ArnonZ commented Nov 13, 2019

Copy link
Copy Markdown

Upgrade to react-native 0.61.4

@ArnonZ ArnonZ requested a review from d4vidi November 13, 2019 13:01
Comment thread lib/src/InteractableView.js Outdated
}
// componentWillMount() {
// // this.chokeTheBridge();
// }

@d4vidi d4vidi Nov 13, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

delete (or restore)?

@ArnonZ ArnonZ Nov 13, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think this should be kept in place for future reference due to this method's nature - Updated the comment to make its usage clearer.

Comment thread package.json Outdated
"detox": "^12.0.0",
"eslint": "^5.16.0",
"jest": "^24.5.0",
"jetifier": "^1.6.4",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is a one-shot dep, isn't it? I think you can remove it

Comment thread package.json Outdated
"@react-native-community/eslint-config": "^0.0.3",
"@types/react-native": ">= 0.52.2",
"babel-jest": "^24.5.0",
"detox": "^12.0.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

^14.0.0

Comment thread package.json Outdated
},
"dependencies": {},
"dependencies": {
"react-native-webview": "^7.4.3"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed. Was indeed unnecessary.

* give correct results when using with locales other than en-US. Note that
* this variant is about 6MiB larger per architecture than default.
*/
def djscFlavor = 'org.webkit:android-jsc:+'

@d4vidi d4vidi Nov 13, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you can get rid of all'a this - both the var and the typo

} catch (InvocationTargetException e) {
e.printStackTrace();
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

removeeeee

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It cannot be removed. Android build fails without this since InvocationTargetException must be caught.

<!-- Base application theme. -->
<style name="AppTheme" parent="Theme.AppCompat.Light.NoActionBar">
<!-- Customize your theme here. -->
<item name="android:textColor">#000000</item>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this on purpose?

@ArnonZ ArnonZ Nov 13, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

On purpose? yes, as part of the official RN migration guide.
Adds real value? meh.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

try to see if the app looks decent without it and if so, better remove... better not custom-theme if no need to

Comment thread playground/android/build.gradle Outdated
classpath 'com.android.tools.build:gradle:3.3.1'
classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:1.3.0"
classpath('com.android.tools.build:gradle:3.4.2')
classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:1.3.31"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

replace 1.3.31 with $kotlinVersion

Comment thread playground/android/build.gradle Outdated
compileSdkVersion = 28
targetSdkVersion = 28
supportLibVersion = "28.0.0"
kotlinVersion = '1.3.31'

@d4vidi d4vidi Nov 13, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

indention

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I like it when you're rough...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

😻

Comment thread playground/android/settings.gradle Outdated
@@ -1,5 +1,5 @@
rootProject.name = 'playground'

apply from: file("../../node_modules/@react-native-community/cli-platform-android/native_modules.gradle"); applyNativeModulesSettingsGradle(settings)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not needed

Comment thread .gitignore
.gradle
local.properties
android/.project
android/.settings/*

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this really needed?

@ArnonZ ArnonZ Nov 13, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, these files should not be committed nor their changes tracked.

Comment thread .gitignore
android/app/libs
android/keystores/debug.keystore
*.keystore
!debug.keystore

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

im not sure about this one

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Was added by RN official migration guide.

@ArnonZ ArnonZ requested a review from d4vidi November 13, 2019 22:18
@oliverdolgener

Copy link
Copy Markdown

Looks good to me. Hopefully this will get merged

@ethanshar ethanshar merged commit 944625f into master Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants