Skip to content

Conversation

MatiPl01
Copy link
Member

@MatiPl01 MatiPl01 commented Aug 11, 2025

Summary

This PR removes unused viewTag prop from the getViewProp function which is a leftover from the old Paper-compatible implementation.

Test plan

I tested it on the getViewProp example (which for some reason was disabled before on Fabric, likely because of the invalid getViewProp implementation).

Screen.Recording.2025-08-12.at.00.26.35.mp4

// @ts-ignore this is fine
const viewTag = animatedRef() as number;
const result = await getViewProp(viewTag, 'opacity', animatedRef.current);
const result = await getViewProp(animatedRef.current, 'opacity');
Copy link
Member Author

Choose a reason for hiding this comment

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

I know that this params order change is a breaking change but, since the viewTag was the first param, I had to move all params (and I took liberty of changing the order of params to get the component as the first param and the prop name as the second one).

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just pass animatedRef instead of animatedRef.current?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is how it was working before - we were passing a component instance instead of a ref object. It will be also a bit harder to type this method properly if we wanted to pass a ref (it should accept React's ref, as well as, our animated ref)

Copy link
Member Author

Choose a reason for hiding this comment

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

I talked with @tjzel and we decided to add method on the animated ref for easier usage. The old API will remain unchanged for backwards compatibility.

Base automatically changed from @matipl01/animated-ref-type-improvement to main August 12, 2025 14:50
@MatiPl01 MatiPl01 force-pushed the @matipl01/remove-unused-getViewProp-param branch from 1973991 to d5345d9 Compare August 14, 2025 08:52
@MatiPl01 MatiPl01 requested a review from tjzel August 14, 2025 08:53
@MatiPl01 MatiPl01 force-pushed the @matipl01/remove-unused-getViewProp-param branch from d5345d9 to 4632dfc Compare August 14, 2025 10:03
// @ts-ignore this is fine
const viewTag = animatedRef() as number;
const result = await getViewProp(viewTag, 'opacity');
const result = await animatedRef.getViewProp<number>('opacity');
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we want getViewProp to be in the public API of Reanimated. This method is meant for runtime tests and should not be used by apps as far as I know.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mhm, it was exported as a function but not documented, so maybe you are right that this was meant only for the internal use. What should I do then if it is not a part of the public API? Can I restore previous changes that just removed the unused param from the function signature and not care about backwards compatibility?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's not a part of the public API then we can import it from src/ directly in the runtime tests, instead of putting it in public exports.

Copy link
Member

Choose a reason for hiding this comment

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

  1. Let's not care about backwards compatibility
  2. Let's convert it from ref method back to a standalone function
  3. I'm okay with moving it to src/ directory

@piaskowyk piaskowyk self-requested a review August 22, 2025 14:39
Copy link
Member

@piaskowyk piaskowyk left a comment

Choose a reason for hiding this comment

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

Generally, it looks okay, but I agree with Tomek's suggestions. Let's adjust to Tomek's comments from here - and we'll be good to go.

@MatiPl01 MatiPl01 self-assigned this Sep 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants