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

Fix RCTLinkingManager Swift representation (take 2!) #24507

Closed
wants to merge 2 commits into from

Conversation

radex
Copy link
Contributor

@radex radex commented Apr 18, 2019

Summary

The fix in #22764, unfortunately, does not work (at least not in Xcode 10.2 / Swift 5 mode). __has_include(<UIKitCore/UIUserActivity.h>) confuses the ObjC→Swift header converter and makes everything be Any (again, causing a crash at runtime).

This condition was added by @cpojer , so I assume it is important for Facebook tooling, but perhaps there's a different way to satisfy this?

Changelog

[CATEGORY] [TYPE] - Message

Test Plan

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 18, 2019
@pull-bot
Copy link

pull-bot commented Apr 18, 2019

Messages
📖

📋 Changelog Format - Did you include a Changelog? A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 4149c12

@radex
Copy link
Contributor Author

radex commented Apr 18, 2019

Will include a changeling — but for now, I'm waiting for @cpojer's feedback – details above :)

@cpojer
Copy link
Contributor

cpojer commented Apr 18, 2019

Yeah I had to make this change at FB, otherwise it would break and not work. We use this check in other places of the codebase so I thought it is fine here too. I'm open to other suggestions.

@radex
Copy link
Contributor Author

radex commented Apr 18, 2019

OK, I have one idea in mind — will test it and let you know

@cpojer
Copy link
Contributor

cpojer commented Apr 24, 2019

@radex did you have a chance to get back to this?

@radex
Copy link
Contributor Author

radex commented Apr 24, 2019

Not quite :( I'm stuck with this — for some reason, the swift headers converter gets very confused by __has_include...

Can you give me a little more information about how the lack of __has_include is a problem with Facebook's internal tooling? Is it related to BUCK?

Perhaps there's some preprocessor flag that's passed in Facebook builds that are not applicable to open-source builds (e.g. FACEBOOK)? If so, maybe I could use a hack like this:

#ifdef FACEBOOK
#define xcode10_headers_available __has_include......
#elseif
#define xcode10_headers_available true
#endif

@cpojer
Copy link
Contributor

cpojer commented Apr 24, 2019

There is some build system stuff going on where I specifically need to test for that file to make something pass internally. I'm sorry I can't share more details than that :(

@RubenSandwich
Copy link
Contributor

@radex Can you share a working example of this crash? I have some time and want to help you out here.

@cpojer
Copy link
Contributor

cpojer commented Apr 26, 2019

We are trying to fix this internally, a commit should hopefully land and sync out to GitHub soon removing the __has_include.

@cpojer cpojer closed this Apr 26, 2019
facebook-github-bot pushed a commit that referenced this pull request Jun 6, 2022
Summary:
This sync includes the following changes:
- **[dd4950c90](facebook/react@dd4950c90 )**: [Flight] Implement useId hook ([#24172](facebook/react#24172)) //<Josh Story>//
- **[26a5b3c7f](facebook/react@26a5b3c7f )**: Explicitly set `highWaterMark` to 0 for `ReadableStream` ([#24641](facebook/react#24641)) //<Josh Larson>//
- **[aec575914](facebook/react@aec575914 )**: [Fizz] Send errors down to client ([#24551](facebook/react#24551)) //<Josh Story>//
- **[a2766387e](facebook/react@a2766387e )**: [Fizz] Improve text separator byte efficiency ([#24630](facebook/react#24630)) //<Josh Story>//
- **[f7860538a](facebook/react@f7860538a )**: Fix typo in useSyncExternalStore main entry point error ([#24631](facebook/react#24631)) //<François Chalifour>//
- **[1bed20731](facebook/react@1bed20731 )**: Add a module map option to the Webpack Flight Client ([#24629](facebook/react#24629)) //<Sebastian Markbåge>//
- **[b2763d3ea](facebook/react@b2763d3ea )**: Move hydration code out of normal Suspense path ([#24532](facebook/react#24532)) //<Andrew Clark>//
- **[357a61324](facebook/react@357a61324 )**: [DevTools][Transition Tracing] Added support for Suspense Boundaries ([#23365](facebook/react#23365)) //<Luna Ruan>//
- **[2c8a1452b](facebook/react@2c8a1452b )**: Fix ignored setState in Safari when iframe is touched ([#24459](facebook/react#24459)) //<dan>//
- **[62662633d](facebook/react@62662633d )**: Remove enableFlipOffscreenUnhideOrder ([#24545](facebook/react#24545)) //<Ricky>//
- **[34da5aa69](facebook/react@34da5aa69 )**: Only treat updates to lazy as a new mount in legacy mode ([#24530](facebook/react#24530)) //<Ricky>//
- **[46a6d77e3](facebook/react@46a6d77e3 )**: Unify JSResourceReference Interfaces ([#24507](facebook/react#24507)) //<Timothy Yung>//
- **[6cbf0f7fa](facebook/react@6cbf0f7fa )**: Fork ReactSymbols ([#24484](facebook/react#24484)) //<Ricky>//
- **[a10a9a6b5](facebook/react@a10a9a6b5 )**: Add test for hiding children after layout destroy ([#24483](facebook/react#24483)) //<Ricky>//
- **[b4eb0ad71](facebook/react@b4eb0ad71 )**: Do not replay erroring beginWork with invokeGuardedCallback when suspended or previously errored ([#24480](facebook/react#24480)) //<Josh Story>//
- **[99eef9e2d](facebook/react@99eef9e2d )**: Hide children of Offscreen after destroy effects ([#24446](facebook/react#24446)) //<Ricky>//
- **[ce1386028](facebook/react@ce1386028 )**: Remove enablePersistentOffscreenHostContainer flag ([#24460](facebook/react#24460)) //<Andrew Clark>//
- **[72b7462fe](facebook/react@72b7462fe )**: Bump local package.json versions for 18.1 release ([#24447](facebook/react#24447)) //<Andrew Clark>//
- **[22edb9f77](facebook/react@22edb9f77 )**: React `version` field should match package.json ([#24445](facebook/react#24445)) //<Andrew Clark>//
- **[6bf3deef5](facebook/react@6bf3deef5 )**: Upgrade react-shallow-renderer to support react 18 ([#24442](facebook/react#24442)) //<Michael サイトー 中村 Bashurov>//

Changelog:
[General][Changed] - React Native sync for revisions bd4784c...d300ceb

jest_e2e[run_all_tests]

Reviewed By: cortinico, kacieb

Differential Revision: D36874368

fbshipit-source-id: c0ee015f4ef2fa56e57f7a1f6bc37dd05c949877
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Linking CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants