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

Updated vendored react-native-webview to v13.2.2 #22913

Merged
merged 3 commits into from
Jun 19, 2023

Conversation

aleqsio
Copy link
Contributor

@aleqsio aleqsio commented Jun 15, 2023

Why

SDK49

How

It needed some changes to the legacy vendoring code. The new version stores native files in separate folders for new and old arch and uses gradle files to combine two folders.

Test Plan

Tested manually on both platforms.

Checklist

@@ -287,33 +287,35 @@ const config: VendoringTargetConfig = {
`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kudo here's some changes that I'd appreciate a review on

@@ -590,6 +590,24 @@ const vendoredModulesConfig: { [key: string]: VendoredModuleConfig } = {
sourceAndroidPackage: 'com.reactnativecommunity.webview',
targetAndroidPackage: 'versioned.host.exp.exponent.modules.api.components.webview',
},
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kudo same here :)

@aleqsio aleqsio requested a review from Kudo June 15, 2023 13:45
@expo-bot expo-bot added the bot: passed checks ExpoBot has nothing to complain about label Jun 15, 2023
@aleqsio aleqsio force-pushed the @aleqsio/version-react-native-webview-for-sdk49 branch from 97affe5 to b0bee06 Compare June 15, 2023 13:47
find: /@implementation RNCWebViewManager\s*{/,
replaceWith: '$&\n NSString *_scopeKey;',
},
{
paths: 'RNCWebViewManager.m',
paths: 'RNCWebViewManager.mm',
find: '*webView = [RNCWebView new];',
replaceWith: '*webView = [RNCWebView new];\n webView.scopeKey = _scopeKey;',
Copy link
Contributor

Choose a reason for hiding this comment

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

please help to double check all these transforms are updated correctly. the [RNCWebView new] seems not there in the current code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, missed that, will double check to see if all transforms got updated.

Perhaps we should make the vendoring process error out if a transform never got applied?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should make the vendoring process error out if a transform never got applied?

sounds good 👍

@aleqsio aleqsio requested a review from Kudo June 16, 2023 11:55
@aleqsio
Copy link
Contributor Author

aleqsio commented Jun 16, 2023

Thank you for the catch @Kudo, I've checked that all code transformations for iOS get applied correctly.

Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

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

I'm impressed how this package migrates to Kotlin and also back to Java at the same time. RNCWebViewPackage has been migrated from Kotlin to Java, there are a few new files in Java and two new files in Kotlin 😂

@aleqsio aleqsio merged commit e89a8b7 into main Jun 19, 2023
@aleqsio aleqsio deleted the @aleqsio/version-react-native-webview-for-sdk49 branch June 19, 2023 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants