-
Notifications
You must be signed in to change notification settings - Fork 0
ios new arch compatibility #24
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
base: 3.19.4-discord
Are you sure you want to change the base?
Conversation
| layoutAnimationsManager_->transferConfigFromNativeID(nativeId, tag); | ||
| } catch (std::invalid_argument) { | ||
| } catch (std::out_of_range) { | ||
| } catch (...) { |
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.
This needs further investigation. In the monorepo this is throwing an invalid_argument exception but not being caught.
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.
yeah i remember we already fixed that once somewhere.
I think the bug was that auto nativeId = stoi(nativeIdString); the nativeid is usually a number, but when the user sets the nativeId prop as a regular text then this can throw, let me find what we did back then …
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.
ah right, the std::invalid_argument should catch this but in the past didn't due to misconfigured rtti and exceptions flags:
we should double check if we did regress that?
| //std::function<std::optional<std::unique_ptr<int[]>>(std::vector<int> &)>; | ||
| auto preserveMountedTagsFunction = [](std::vector<int> &tags) -> std::optional<std::unique_ptr<int[]>> { | ||
| // TODO: Implement me | ||
| return nullptr; |
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.
This didn't seem to be called by ios new arch and can maybe be left as a noop.
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.
Yes, thats noop on iOS 👍
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.
just wondering if we can also ifdef android that 🤔 thats how SWM did it in the v4 code:
but i'd also be fine with having this here + a comment 🤷
Getting our fork compatible with ios new arch, still some not so great stuff in here that needs to be cleaned up