-
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
Fix universal links not working in iOS 12 / Xcode 10 #22764
Conversation
@hramos Can we merge this? I was about to submit a nearly identical patch, because the app will Crash if you try to call this from Swift (as the type annotations don't match and a dumb cast will fail) |
@radex any chance you can test this on iOS 11 or earlier? At a glance it seems like the incomplete test plan might have been the reason this was not merged back when it was submitted. |
Following snippet is from firebase dynamic links implementation - (BOOL)application:(UIApplication *)application
continueUserActivity:(nonnull NSUserActivity *)userActivity
restorationHandler:
#if defined(__IPHONE_12_0) && (__IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_12_0)
(nonnull void (^)(NSArray<id<UIUserActivityRestoring>> *_Nullable))restorationHandler {
#else
(nonnull void (^)(NSArray *_Nullable))restorationHandler {
#endif // __IPHONE_12_0
// Other Code
} They target ios 12+ specifically to apply this change, I can update my PR to this tomorrow, it should fall back to old implementation on earlier versions of iOS. Sounds ok? |
@iljadaderko Right! I'm not sure if checking for __IPHONE_12_0 is the right way to go — I'd check for other RN sources to check what's the convention for checking for version (so things like tvOS are also taken into account)... there might already be a macro for that or something. But in general, a preprocessor check makes sense. |
@radex I just looked through few react-native-community repos, but can't seem to find any macro for iOS version check / tvOS. Do you have any repos in mind that could use them for me to reference? Perhaps I should go ahead with similar solution firebase use for time being? |
@iljadaderko I did a quick search and this seems to be the RN convention:
(just the defined part, os/log has nothing to do with this) |
If it compiles, looks good to me! @iljadaderko Have you had the chance to check on iOS 11 (Xcode 9) if it still compiles correctly there, just to be sure? |
@radex Only tested this with latest XCode and iOS I'm afraid. Tests seem to be failing, but for things unrelated to this PR? I can try to compile on earlier iOS versions devices latter this week if no one else can do this atm. |
@iljadaderko please do test on a device with an older version of iOS and report back so we have confidence before merging this PR :) |
@cpojer @radex Hey guys, I don't have access to ios 11 device, is anyone able to test this? Back when I replied I had simulator with ios11 in the back of my head, but obviously can't test linking in simulator :/ Tests are now failing as well, but it doesn't look like this is related to linking changes. |
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 think this should be fine to ship.
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.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request was successfully merged by @iljadaderko in 56679ed. When will my fix make it into a release? | Upcoming Releases |
Hey @iljadaderko I finally managed to land this PR. I had to add a check |
Summary: This sync includes the following changes: - **[c1220ebdd](facebook/react@c1220ebdd )**: treat empty string as null ([#22807](facebook/react#22807)) //<salazarm>// - **[09d9b1775](facebook/react@09d9b1775 )**: Update deprecated features in ESLint configuration files. ([#22767](facebook/react#22767)) //<Esteban>// - **[bddbfb86d](facebook/react@bddbfb86d )**: Revert "Fix Node package.json ./ exports deprecation warning ([#22783](facebook/react#22783))" ([#22792](facebook/react#22792)) //<Sebastian Silbermann>// - **[b831aec48](facebook/react@b831aec48 )**: chore(fast-refresh): double check wasMounted ([#22740](facebook/react#22740)) //<anc95>// - **[8edeb787b](facebook/react@8edeb787b )**: Fix Node package.json ./ exports deprecation warning ([#22783](facebook/react#22783)) //<Rin Arakaki>// - **[fdc1d617a](facebook/react@fdc1d617a )**: Flag for client render fallback behavior on hydration mismatch ([#22787](facebook/react#22787)) //<salazarm>// - **[aa19d569b](facebook/react@aa19d569b )**: Add test selectors to experimental build ([#22760](facebook/react#22760)) //<Brian Vaughn>// - **[520ffc77a](facebook/react@520ffc77a )**: Use globalThis if possible for native fetch in browser build ([#22777](facebook/react#22777)) //<Jiachi Liu>// - **[afbc2d08f](facebook/react@afbc2d08f )**: Remove unused react-internal/invariant-args ESLint rule. ([#22778](facebook/react#22778)) //<Esteban>// - **[ca94e2680](facebook/react@ca94e2680 )**: Remove 'packages/shared/invariant.js' ([#22779](facebook/react#22779)) //<Esteban>// - **[83564712b](facebook/react@83564712b )**: Move SuspenseList to experimental channel ([#22765](facebook/react#22765)) //<Andrew Clark>// - **[d4144e6e5](facebook/react@d4144e6e5 )**: fix : grammatical typo for test description ([#22764](facebook/react#22764)) //<Brijesh Prasad>// - **[0b329511b](facebook/react@0b329511b )**: chore: fix comment typo ([#22657](facebook/react#22657)) //<Han Han>// - **[e6f60d2ad](facebook/react@e6f60d2ad )**: fix typos ([#22715](facebook/react#22715)) //<180909>// Changelog: [General][Changed] - React Native sync for revisions c0c71a8...c1220eb jest_e2e[run_all_tests] Reviewed By: yungsters Differential Revision: D32646433 fbshipit-source-id: c534ee7a17141634700c90fc2c7b34bfbe17887a
Summary: This sync includes the following changes: - **[c1220ebdd](facebook/react@c1220ebdd )**: treat empty string as null ([facebook#22807](facebook/react#22807)) //<salazarm>// - **[09d9b1775](facebook/react@09d9b1775 )**: Update deprecated features in ESLint configuration files. ([facebook#22767](facebook/react#22767)) //<Esteban>// - **[bddbfb86d](facebook/react@bddbfb86d )**: Revert "Fix Node package.json ./ exports deprecation warning ([facebook#22783](facebook/react#22783))" ([facebook#22792](facebook/react#22792)) //<Sebastian Silbermann>// - **[b831aec48](facebook/react@b831aec48 )**: chore(fast-refresh): double check wasMounted ([facebook#22740](facebook/react#22740)) //<anc95>// - **[8edeb787b](facebook/react@8edeb787b )**: Fix Node package.json ./ exports deprecation warning ([facebook#22783](facebook/react#22783)) //<Rin Arakaki>// - **[fdc1d617a](facebook/react@fdc1d617a )**: Flag for client render fallback behavior on hydration mismatch ([facebook#22787](facebook/react#22787)) //<salazarm>// - **[aa19d569b](facebook/react@aa19d569b )**: Add test selectors to experimental build ([facebook#22760](facebook/react#22760)) //<Brian Vaughn>// - **[520ffc77a](facebook/react@520ffc77a )**: Use globalThis if possible for native fetch in browser build ([facebook#22777](facebook/react#22777)) //<Jiachi Liu>// - **[afbc2d08f](facebook/react@afbc2d08f )**: Remove unused react-internal/invariant-args ESLint rule. ([facebook#22778](facebook/react#22778)) //<Esteban>// - **[ca94e2680](facebook/react@ca94e2680 )**: Remove 'packages/shared/invariant.js' ([facebook#22779](facebook/react#22779)) //<Esteban>// - **[83564712b](facebook/react@83564712b )**: Move SuspenseList to experimental channel ([facebook#22765](facebook/react#22765)) //<Andrew Clark>// - **[d4144e6e5](facebook/react@d4144e6e5 )**: fix : grammatical typo for test description ([facebook#22764](facebook/react#22764)) //<Brijesh Prasad>// - **[0b329511b](facebook/react@0b329511b )**: chore: fix comment typo ([facebook#22657](facebook/react#22657)) //<Han Han>// - **[e6f60d2ad](facebook/react@e6f60d2ad )**: fix typos ([facebook#22715](facebook/react#22715)) //<180909>// Changelog: [General][Changed] - React Native sync for revisions c0c71a8...c1220eb jest_e2e[run_all_tests] Reviewed By: yungsters Differential Revision: D32646433 fbshipit-source-id: c534ee7a17141634700c90fc2c7b34bfbe17887a
fix #22716
Changelog:
[iOS][Fixed] - Fix universal links in iOS 12 / Xcode 10
Test Plan:
Tested this on a real device running iOS 12, in theory it should work on older versions as well as previous implementation assumed
*
type forrestorationHandler
params, however if someone could test this on iOS 11 and lower, it would be much appreciated.