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

Improve TypeSafety #16

Merged
merged 13 commits into from
Jul 16, 2020
Merged

Improve TypeSafety #16

merged 13 commits into from
Jul 16, 2020

Conversation

META-DREAMER
Copy link
Contributor

Right now a lot of type information is lost when passing things between functions, most importantly the type of the users theme which might not have the exact same shape as the BaseTheme. This updates many of the typings to use generics which helps enforce type safety within the restyle codebase as well as for the end users (e.g. ensuring the themeKey they pass in is a valid one from their Theme, or the property they pass actually exists on the Props for the component).

The TS versions and ESLint config was pretty outdated, it didn't have support for the Record type, so I updated to the newer @shopify/eslint-plugin as well which is compatible with TS v3.7.5.

@ghost ghost added the cla-needed label Jun 26, 2020
property,
transform,
styleProperty = property,
styleProperty = property.toString(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since objects can have numbered or Symbol properties, but the styleProperty type is a string, we need to convert it to a string if we want to assign it like this by default.

@META-DREAMER
Copy link
Contributor Author

Signed the CLA, need to re-run the check

@JoelBesada
Copy link
Contributor

Hi Hammad, thank you so much for taking the time to contribute to this library! I'm currently on vacation until July 13, but I'll see if I can get someone else on the team to go through your PR before that.

No longer necessary since TS includes this out of the box
This ensures that the output props from useRestyle are actually typed properly instead of casting them to `any`
…ng the restyleFunctions param to be typesafe
Enforces correct values for property, themeKey and the return type
Copy link
Contributor

@JoelBesada JoelBesada left a comment

Choose a reason for hiding this comment

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

This looks great, just one question. I would also appreciate if you could update the themeKey param of createVariant in a similar fashion 🙏

const createRestyleFunction = <
TProps = Record<string, unknown>,
Theme extends BaseTheme = BaseTheme,
P extends keyof TProps = keyof TProps,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a valid case for when you should be able to override P and K here? If not, I would prefer to just using keyof TProps and keyof Theme inline within the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not so much about overriding as it is about having a strongly typed return type so that the exact property and themeKey the user specified is preserved and carried through to anything consuming that restyle function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, got it

@ghost ghost removed the cla-needed label Jul 14, 2020
@META-DREAMER
Copy link
Contributor Author

META-DREAMER commented Jul 14, 2020

This looks great, just one question. I would also appreciate if you could update the themeKey param of createVariant in a similar fashion 🙏

@JoelBesada Done. Also updated useRestyle with better typings.

Note, I also have more improvements that build on this PR, but would like to get this merged so it doesn't get too big. You can see those PRs here if you're curious: https://github.com/hammadj/restyle/pulls

const createVariant = <
Theme extends BaseTheme,
TProps extends VariantProps<Theme, K, P> = VariantProps<Theme, keyof Theme>,
P extends keyof TProps | 'variant' = 'variant',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the P type is being set too strictly here, I'm not allowed to use anything other than 'variant' as the prop name.
image

This should be able to accept any string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch, have to make a small fix to allow other prop names, if the Prop types aren't defined, but it would still be good to enforce the type safety if the user does pass in props. Should work like this now:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoelBesada Just pushed some changes giving createVariant some 🔥 type safety and inference. Try it out and let me know how it works. Stress test it with custom properties and PropTypes and themes, etc. It should be able to pick up any incorrect setup no matter how/where it's used. It works best when defined inline with createRestyleComponent/useRestyle, etc, but it still works when its not.

This separates the types for createVariant into two overloads, one for when a custom property is defined and one when its not.
The result of this is perfect inference of types when used in createRestyleComponent while not requiring the user to define
the generic types if they dont want to.
Copy link
Contributor

@JoelBesada JoelBesada left a comment

Choose a reason for hiding this comment

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

Great work on this! Merging this into master now, a new version including this will be published to NPM some time next week most likely.

@JoelBesada JoelBesada merged commit 19ec0a8 into Shopify:master Jul 16, 2020
JoelBesada added a commit that referenced this pull request Jul 16, 2020
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.

2 participants