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

Name every worklet function #6163

Merged
merged 15 commits into from
Jul 16, 2024
Merged

Name every worklet function #6163

merged 15 commits into from
Jul 16, 2024

Conversation

tjzel
Copy link
Collaborator

@tjzel tjzel commented Jun 24, 2024

Summary

Currently, in case of unnamed worklets, such as

() => {
  'worklet';
  // some logic
}

function () {
  'worklet'
  // some logic
}

Those functions are named anonymous and parsed as such on the Worklet Runtime. However, this is a pitfall for Reanimated. When the anonymous function crashes and the stack trace is vague - it usually is, especially in production - the first suspicion is the error in Reanimated's core logic, since we don't know which function in particular has failed.

In case of a faulty logic in a third-party library this is double as harmful. It doesn't lead the creators of these libraries into the pit of success, since they might not be aware it's their code causing issues. Conversely, Reanimated maintainers receive a very obscure issue and engage in rather unpleasant debugging process - only to discover later the library's fault.

My idea is to:

  1. Add source library name to every worklet function.
  2. Add source file name to every worklet function.
  3. Add ID to every worklet function.

Test plan

  • Update the plugin test snapshots.
  • Add relevant test cases if not present already.
  • Try to recreate a crash for a third party library, compare the clarity of raised exceptions.
BeforeAfter
Production developer environment
Screenshot 2024-06-26 at 11 26 01
Sentry stack trace

Sentry stack traces are coming from Android. Unfortunately, regardless of these changes iOS Sentry stack traces look like this:

Screenshot 2024-06-26 at 12 42 24

@tjzel tjzel marked this pull request as ready for review July 2, 2024 08:29
@tjzel tjzel added this pull request to the merge queue Jul 16, 2024
Merged via the queue into main with commit a6dc6b4 Jul 16, 2024
7 checks passed
@tjzel tjzel deleted the @tjzel/plugin/named-worklets branch July 16, 2024 14:44
@m-bert m-bert mentioned this pull request Jul 22, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 22, 2024
## Summary

After #6163 was merged, each `worklet` function follows new naming
convention. Since `easings` are `worklets`, their `name` property has
also been changed. In web layout animations we were applying `easing`
based on its name, therefore changing it resulted in only default
`linear` easing being available on web.

This PR adds new `Symbol` into easing functions so that we can once
again get their name and apply correct easing to animations.

## Test plan

Tested on _**BasicLayoutAnimation**_ example with `Easing.exp`
@tjzel tjzel mentioned this pull request Aug 23, 2024
2 tasks
github-merge-queue bot pushed a commit that referenced this pull request Aug 28, 2024
## Summary

Turns out that in #6163 I overlooked the fact that we shouldn't rename
React part of worklet functions. This PR changes the naming behavior,
only worklet counterparts are renamed for better error verbosity. See
differences in snapshots for more details.

## Test plan

- [x] Upgraded jest unit tests pass
- [x] New runtime tests pass
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