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

Add runOnJS types #1392

Merged
merged 5 commits into from
Nov 2, 2020
Merged

Add runOnJS types #1392

merged 5 commits into from
Nov 2, 2020

Conversation

karol-bisztyga
Copy link
Contributor

@karol-bisztyga karol-bisztyga commented Oct 30, 2020

Description

  • add runOnJS typescript types
  • fix link in docs' installation section
  • interpolateColor - thanks to @mrousavy
    • add TS types tests
    • add TS types
    • replace enum with string union

@mrousavy
Copy link
Contributor

I've also noticed that types for interpolateColor are missing, do you want me to create a PR or are you on it?

@karol-bisztyga
Copy link
Contributor Author

I've also noticed that types for interpolateColor are missing, do you want me to create a PR or are you on it?

Hey, I've added types for interpolateColor, you can check if they're valid, thanks for pointing this out!

@mrousavy
Copy link
Contributor

Looks good, I think color space can be optional, no?

Also on a side note, I'd recommend not using enums. Consider the following:

import { ColorSpace } from '...';
const x = interpolateColor(y, [0, 1], ['#fff', '#000'], ColorSpace.RGB);

which would so much cleaner if it were written like this:

const x = interpolateColor(y, [0, 1], ['#fff', '#000'], 'rgb');

Same applies for the Extrapolate prop, I prefer writing

{ extrapolate: 'clamp' }

to

{ extrapolate: Extrapolate.CLAMP }

as this way I don't need an extra import, don't need to wait for my IDE (or typescript) to show me what values Extrapolate has (after it has imported it), and I don't have to repeat myself. Also see: Why TypeScript enums suck

This doesn't have anything to do with this PR in particular, thought I'd just point this out to consider this before reanimated v2 is initially released, as breaking changes wouldn't be a problem in an alpha.

Thoughts? @karol-bisztyga @Szymon20000 @kmagiera @wcandillon @terrysahaidak

@jakub-gonet
Copy link
Member

jakub-gonet commented Oct 30, 2020

@mrousavy I think this discussion should be moved into a new issue.

Agreed, string unions are way cleaner and I'm pretty sure no one wants runtime behavior of enums (e.g. enumerating possible values) but I may be wrong.

Looks good, I think color space can be optional, no?

Definitely, we default to RGB space in JS.

@karol-bisztyga
Copy link
Contributor Author

@mrousavy thanks for the review, I updated the code

@karol-bisztyga karol-bisztyga merged commit cd0b0e2 into master Nov 2, 2020
@karol-bisztyga karol-bisztyga deleted the @karol/run-on-js-types branch November 2, 2020 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants