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

Reanimated doesn't compile with React Native macOS 0.75 #6501

Open
Saadnajmi opened this issue Sep 11, 2024 · 10 comments
Open

Reanimated doesn't compile with React Native macOS 0.75 #6501

Saadnajmi opened this issue Sep 11, 2024 · 10 comments
Assignees
Labels
Missing repro This issue need minimum repro scenario

Comments

@Saadnajmi
Copy link

Description

Someone on discord reported that Reanimated doesn't compile with React Native macOS 0.75, specifically on this line:

addUIBlock:^(__unused RCTUIManager *manager, __unused NSDictionary<NSNumber *, REAUIView *> *viewRegistry) {

I think the issue is because of this:

In earlier versions React Native macOS, we defined the view registry as NSDictionary<NSNumber *, RCTUIView *> *viewRegistry, where RCTUIView is a subclass of NSView that overrides and adds some methods for easier compatibility with iOS. In React Native macOS 0.75 (and maybe 0.74?), we changed the definition to NSDictionary<NSNumber *, RCTPlatformView *> *viewRegistry, where RCTPlatformView is simply a typedef of either NSView or UIView.

I thought the change would be innocuous since we were widening the scope of what views were accepted into the view registry, but I guess not? Said person on discord can get around the issue by replacing REAUIView with RCTPlatformView wherever there's an error.

I'm not entirely sure whether the fix should be React Native macOS reverts the change I described above, or we update Reanimated to use something like REAPlatformView. My main concern is whether or not updating Reanimated keeps it backwords compatible with earlier versions of React Native macOS.

Steps to reproduce

Try to build a new macOS test app (initialized with react-native-macos-init) with Reanimated.

Snack or a link to a repository

I'm copy pasta-ing info someone sent me on discord.. so I don't have one yet.

Reanimated version

the latest?

React Native version

0.75

Platforms

macOS

JavaScript runtime

None

Workflow

None

Architecture

None

Build type

None

Device

None

Device model

No response

Acknowledgements

Yes

@github-actions github-actions bot added the Missing repro This issue need minimum repro scenario label Sep 11, 2024
@Saadnajmi
Copy link
Author

I'll add the image from Discord as well for minimal repro:
image

@BlackCatTB
Copy link

Thanks absolute legend!

@BlackCatTB
Copy link

I found this is a quick fix when you replace the following lines with these code blocks:

Files and Fixes:


REANNodesManager

Lines 575-594

[_uiManager
    addUIBlock:^(__unused RCTUIManager *manager, __unused NSDictionary<NSNumber *,
#if TARGET_OS_OSX
    RCTPlatformView
#else
    REAUIView
#endif
    *> *viewRegistry) {
        __typeof__(self) strongSelf = weakSelf;
        if (strongSelf == nil) {
            return;
        }
        atomic_store(&strongSelf->_shouldFlushUpdateBuffer, false);
        NSMutableDictionary *componentUpdateBuffer = [strongSelf->_componentUpdateBuffer copy];
        strongSelf->_componentUpdateBuffer = [NSMutableDictionary new];
        for (NSNumber *tag in componentUpdateBuffer) {
            ComponentUpdate *componentUpdate = componentUpdateBuffer[tag];
            if (componentUpdate == Nil) {
                continue;
            }
            [strongSelf updateProps:componentUpdate.props
                      ofViewWithTag:componentUpdate.viewTag
                           withName:componentUpdate.viewName];
        }
        [strongSelf performOperations];
}];

REASwizzeledUIManager

Line 235

#if TARGET_OS_OSX
    return ^(__unused RCTUIManager *uiManager, NSDictionary<NSNumber *, RCTPlatformView *> *viewRegistry) {
#else
    return ^(__unused RCTUIManager *uiManager, NSDictionary<NSNumber *, REAUIView *> *viewRegistry) {
#endif

Lines 379-382

[self reanimated_addUIBlock:^(
    __unused RCTUIManager *manager, __unused NSDictionary<NSNumber *,
#if TARGET_OS_OSX
    RCTPlatformView
#else
    REAUIView
#endif
    *> *viewRegistry) {
        --isFlushingBlocks;
}];

REAModule

Lines 239-245

[uiManager
    addUIBlock:^(__unused RCTUIManager *manager, __unused NSDictionary<NSNumber *,
#if TARGET_OS_OSX
    RCTPlatformView
#else
    REAUIView
#endif
    *> *viewRegistry) {
        for (AnimatedOperation operation in operations) {
            operation(nodesManager);
        }
        [nodesManager operationsBatchDidComplete];
    }];

RNGestureHandler

Lines 271-276

[uiManager
    addUIBlock:^(__unused RCTUIManager *manager, __unused NSDictionary<NSNumber *,
#if TARGET_OS_OSX
    RCTPlatformView
#else
    RNGHUIView
#endif
    *> *viewRegistry) {
        for (GestureHandlerOperation operation in operations) {
            operation(self->_manager);
        }
    }];

These fixes introduce platform-specific checks using #if TARGET_OS_OSX to ensure compatibility between macOS and iOS.

Thank you!

@m-bert
Copy link
Contributor

m-bert commented Sep 16, 2024

Hi @Saadnajmi! Thank you for reporting this issue.

Was there a specific reason to change RCTUIView to RCTPlatformView? I've tried to change our REAUIView definition into RCTPlatformView, but it restrains us from accessing fields defined in RCTUIView (e.g. transform or backgroundColor).

A potential solution in our codebase would be to create new type:

#if TARGET_OS_OSX
typedef NSDictionary<NSNumber *, RCTPlatformView *> ViewRegistry;
#else
typedef NSDictionary<NSNumber *, RCTUIView *>  ViewRegistry;
#endif

and then using ViewRegistry. However, we believe that it would be better if you could revert this change in react-native-macos if possible.

@hsjoberg
Copy link

This looks like a dupe of #6421.

I did make some quick fixes for reanimated and gesture handler here:

https://github.com/hsjoberg/react-native-reanimated/tree/rn75fix
https://github.com/hsjoberg/react-native-gesture-handler/tree/rn75fix

But perhaps something like REAPlatformView is more appropriate.

@github-actions github-actions bot added Repro provided A reproduction with a snippet of code, snack or repo is provided and removed Missing repro This issue need minimum repro scenario labels Sep 16, 2024
@Saadnajmi
Copy link
Author

@m-bert I was hoping it would be a non-breaking change that just widened the view registry. I assume we can't guarantee only RCTUIView's will be there. But it's true that transform and backgroundColor are hard to access. I'll do some more investigating before I revert the change.

@github-actions github-actions bot added the Missing repro This issue need minimum repro scenario label Sep 16, 2024
Copy link

Hey! 👋

The issue doesn't seem to contain a minimal reproduction.

Could you provide a snack or a link to a GitHub repository under your username that reproduces the problem?

@github-actions github-actions bot removed the Repro provided A reproduction with a snippet of code, snack or repo is provided label Sep 16, 2024
@Saadnajmi
Copy link
Author

@m-bert I did some testing and it seems React Native Gesture Handler has the same issues with my change, so I'm more likely to revert.
A question: I see that Reanimated extended RCTUIView to add a center prop to match iOS. If I upstream that to React Native macOS (and back port to say... 0.73), would that be good fro you? I'm worried about "duplicate property" style issues, but if I back port the change to the minimum RN version you support and then remove, I think it'll be OK?

@Saadnajmi
Copy link
Author

@m-bert I found the fix! I need to add __kindof to RCTViewManagerUIBlock. AKA:

typedef void (^RCTViewManagerUIBlock)(RCTUIManager *uiManager, NSDictionary<NSNumber *, __kindof RCTPlatformView *> *viewRegistry); // [macOS]

I can do that and backport!

@Saadnajmi
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missing repro This issue need minimum repro scenario
Projects
None yet
Development

No branches or pull requests

4 participants