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

v2 types #840

Merged
merged 44 commits into from Jul 22, 2020
Merged

v2 types #840

merged 44 commits into from Jul 22, 2020

Conversation

renanmav
Copy link
Contributor

Description

This PR includes the type definitions for react-native-reanimated v2. I created these types following the documentation and doing a quick check within the source code. Tested it against some changes I made on my reanimated-v2-playground. A better test would be to use these types on example app.

Suggestions

Increase DX for types, lint-staged isn’t configured to work for typescript files.

react-native-reanimated.d.ts Outdated Show resolved Hide resolved
toValue: number,
userConfig?: Omit<TimingConfig, 'toValue'>,
callback?: (isCancelled: boolean) => void,
): number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this should return a number. Each animation actually returns an object. But maybe @kmagiera have better thoughts on this.

Copy link
Contributor Author

@renanmav renanmav Jun 3, 2020

Choose a reason for hiding this comment

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

I’m doing this to make the imperative compatible with:

  1. SharedValue
  2. react-native styles (ViewStyle, TextStyle, ImageStyle)

Thoughts @kmagiera?

react-native-reanimated.d.ts Outdated Show resolved Hide resolved
react-native-reanimated.d.ts Outdated Show resolved Hide resolved
@renanmav
Copy link
Contributor Author

renanmav commented May 30, 2020

After some local usage of these types (patch-package to enjoy the types), I saw that generics don’t work for useAnimatedGestureHandler and useAnimatedStyle, maybe there is something related to the UI Thread not being able to parse the Typescript type annotation.

I also saw I’ve forgot the interpolation function type.

@terrysahaidak
Copy link
Contributor

After some local usage of these types (patch-package to enjoy the types), I saw that generics don’t work for useAnimatedGestureHandler and useAnimatedStyle, maybe there is something related to the UI Thread not being able to parse the Typescript type annotation.

Yeah, right now typescript annotations aren't getting removed from worklets before babel plugin processing them.

react-native-reanimated.d.ts Outdated Show resolved Hide resolved
@terrysahaidak
Copy link
Contributor

@renanmav actually, Shared Value is not a class in the code, I would just use type/interface here. There is no way to new it :)

Also, as I mentioned before, T extends Value but it' not correct since Value - string | number | boolean. But you can pass also an object, array, and even function here.

@wcandillon
Copy link
Contributor

wcandillon commented Jun 9, 2020

@renanmav Thank you so much for your effort. With now type annotations supported with worklets, things are getting interesting :) I'm also thinking about an eslint plugin to check the env variables used from worklets 😅

I'm trying your definition and added a definition for onRunUI():

    // reanimated2 functions
    export function runOnUI<ReturnType>(fn: () => ReturnType): () => ReturnType;
    export function runOnUI<P1, ReturnType>(fn: (p1: P1) => ReturnType): (p1: P1) => ReturnType;
    export function runOnUI<P1, P2, ReturnType>(fn: (p1: P1, p2: P2) => ReturnType): (p1: P1, p2: P2) => ReturnType;
    export function runOnUI<P1, P2, P3, ReturnType>(fn: (p1: P1, p2: P2, p3: P3) => ReturnType): (p1: P1, p2: P2, p3: P3) => ReturnType;
    export function runOnUI<P1, P2, P3, P4, ReturnType>(fn: (p1: P1, p2: P2, p3: P3, p4: P4) => ReturnType): (p1: P1, p2: P2, p3: P3, p4: P4) => ReturnType;
    export function runOnUI<P1, P2, P3, P4, P5, ReturnType>(fn: (p1: P1, p2: P2, p3: P3, p4: P4, p5: P5) => ReturnType): (p1: P1, p2: P2, p3: P3, p4: P4, p5: P5) => ReturnType;
    export function runOnUI<A, ReturnType>(fn: (...args: A[]) => ReturnType): (...args: A[]) => ReturnType;
//...
  export const runOnUI: typeof Animated.runOnUI

@wcandillon
Copy link
Contributor

wcandillon commented Jun 9, 2020

@terrysahaidak how can we deal with assigning animations to shared values? Here withTiming return value is typed as number. But it is not a number. I'm wondering what is the most elegant to deal with this.

We can do something like this:

    class SharedValue<T extends SharedValueType> {
      value: T;
      set value(value: T | Animation);
    }

Not sure how to define Animation yet.

@renanmav
Copy link
Contributor Author

renanmav commented Jun 9, 2020

I'm trying your definition and added a definition for onRunUI()

I’m not sure what onRunUI means. Would you mind clarify it @wcandillon?

@wcandillon
Copy link
Contributor

runOnUI call a worklet function from the JS thread. You can pass it parameter. You can see an example of it at https://github.com/wcandillon/can-it-be-done-in-react-native/blob/master/reanimated-2/src/Worklets/Worklets.tsx#L53

@renanmav
Copy link
Contributor Author

renanmav commented Jun 9, 2020

So, can I add your runOnUI definition in this PR @wcandillon?

Update react-native-reanimated.d.ts
@wcandillon
Copy link
Contributor

@renanmav Great work! about the CI failure, could it be just a matter of merging the latest master into this branch?

@terrysahaidak Does this work for you as well?

): number;
export function withSpring(
toValue: number,
userConfig?: Omit<SpringConfig, 'toValue'>,
Copy link
Contributor

Choose a reason for hiding this comment

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

SpringConfig is missing velocity: number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 5fd666e

Copy link
Contributor

@terrysahaidak terrysahaidak left a comment

Choose a reason for hiding this comment

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

There is withDecay already in the master.

type DecayConfig = {
  deceleration?: number;
  velocity: number;
  clamp?: [number, number];
};

export function withDecay(
  userConfig: DecayConfig,
  callback: (isCancelled: boolean) => void
): number;

@wcandillon
Copy link
Contributor

Thks for your review @terrysahaidak
On top of that I would merge the proc change from https://github.com/software-mansion/react-native-reanimated/pull/890/files

    export function proc<A extends Adaptable[]>(
      cb: (...args: A) => AnimatedNode<number>
    ): typeof cb;

@renanmav you rock!

@jakub-gonet jakub-gonet linked an issue Jul 9, 2020 that may be closed by this pull request
@terrysahaidak
Copy link
Contributor

@renanmav velocity is not required AFAIK.

@wcandillon
Copy link
Contributor

@renanmav Could you fix the merge conflict?
@jakub-gonet this is good to go.

@wcandillon
Copy link
Contributor

Is a proposal for the merge conflict: https://github.com/renanmav/react-native-reanimated/pull/3/files

@terrysahaidak
Copy link
Contributor

The sequence helper has been added recently. Also, Loop has been renamed to repeat
https://github.com/software-mansion/react-native-reanimated/blob/master/src/reanimated2/animations.js#L332
https://github.com/software-mansion/react-native-reanimated/blob/master/src/reanimated2/animations.js#L371

2. Add repeat function
3. Add sequence function
4. Add test for sequence function
Change loop to repeat and add sequence function
@renanmav
Copy link
Contributor Author

Shall we merge this?

Copy link
Member

@jakub-gonet jakub-gonet left a comment

Choose a reason for hiding this comment

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

I think yes. I'm still not convinced about tests being in the root of the package and adding react-native-gesture-handler as a peer dependency, but we can fix it in the follow-up PR after tests to the Reanimated 2 are set up.

@jakub-gonet
Copy link
Member

🎉

@jakub-gonet jakub-gonet merged commit aa0b739 into software-mansion:master Jul 22, 2020
@wcandillon
Copy link
Contributor

Woah congrats everyone 🙌🏻 @renanmav you're our TypeScript hero 🦸🏻‍♂️

@renanmav renanmav deleted the v2-types branch July 22, 2020 18:30
@MinimogDev
Copy link

Do you guys have an eta on when these will become available in alpha release?

@wcandillon
Copy link
Contributor

wcandillon commented Aug 3, 2020 via email

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.

V2, Typescript support
8 participants