Skip to content

Change logic of worklet detection in config #3745

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

Change logic of worklet detection in config #3745

wants to merge 3 commits into from

Conversation

m-bert
Copy link
Contributor

@m-bert m-bert commented Oct 14, 2025 •
edited
Loading

Description

This PR fixes issue where using Animated.Event without worklet callbacks was throwing error. This was because current implementation of hasWorkletEventHandlers was checking all functions present in config, including changeEventCalculator (which is in fact worklet).

Note

It previously contained #3748

Test plan

Tested on the current basic-example app

@m-bert
Copy link
Contributor Author

m-bert commented Oct 14, 2025

If you prefer to split these changes into separate PRs, let me know.

akwasniewski
akwasniewski approved these changes Oct 14, 2025
View reviewed changes
Copy link
Contributor

@akwasniewski akwasniewski left a comment

Choose a reason for hiding this comment

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

Looks good, though I think it makes sense to split it into two PRs, as fixing Animated.Event is probably a temporary fix, so when we reference it in the future it's going to be cleaner. Should we add a task with a proper fix to the roadmap?

@m-bert m-bert mentioned this pull request Oct 14, 2025
Merged
@m-bert m-bert changed the title Change logic of worklet detection in config & fix Animated.Event Change logic of worklet detection in config Oct 14, 2025
@m-bert m-bert requested a review from akwasniewski October 14, 2025 13:56
@m-bert
Copy link
Contributor Author

m-bert commented Oct 14, 2025

Looks good, though I think it makes sense to split it into two PRs, as fixing Animated.Event is probably a temporary fix, so when we reference it in the future it's going to be cleaner.

Sure, done!

Should we add a task with a proper fix to the roadmap?

We could. The question is whether we will forget about it or not 😄 If there's already a task connected with LogicDetector (I'm not sure if it is marked as done or not), we could add it to description.

@akwasniewski
Copy link
Contributor

akwasniewski commented Oct 14, 2025 •
edited
Loading

If there's already a task connected with LogicDetector (I'm not sure if it is marked as done or not), we could add it to description.

I don't think we ever created one. I guess we might add it to the one with fixing LogicDetector handling of NativeViewGestures

m-bert added a commit that referenced this pull request Oct 14, 2025
@m-bert
Do not use handleGestureEvent for Animated.Event (#3748)
470ed63 
## Description

This PR removes `handleGestureEvent` from
`onGestureHandlerAnimatedEvent`. This function was trying to call
`Animated.Event`, which is not possible since it is an object. This
takes out part of `LogicDetector` logic, however it didn't work anyway
and at least standard `NativeDetector` works as it should.

> [!NOTE]
> It was previously a part of #3745

## Test plan

Tested on current `basic-example`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akwasniewski akwasniewski akwasniewski approved these changes

@j-piasecki j-piasecki Awaiting requested review from j-piasecki

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@m-bert @akwasniewski