adding types for @emotion/native #1634

patsissons · 2019-11-14T20:04:46Z

What:

adds some types for @emotion/native

Why:

How:

Checklist:

Documentation
Tests
Code complete
Changeset

changeset-bot · 2019-11-14T20:04:49Z

🦋 Changeset is good to go

Latest commit: 2f08db6

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codesandbox-ci · 2019-11-14T20:05:57Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2f08db6:

Sandbox	Source
Emotion	Configuration

Andarist · 2019-11-14T21:48:42Z

Thank you for working on this! From what I can see those are based on emotion@10 typings - would you be willing to convert them to what we have currently on the next branch for @emotion/styled?

patsissons · 2019-11-14T22:22:04Z

@Andarist the types are based on the next branch? am i missing something?

Andarist · 2019-11-14T22:50:38Z

For example there are no equivalents for ThemelessReactNativeStyledComponent & ThemedReactNativeStyledComponent in https://github.com/emotion-js/emotion/blob/ad77ed24b4bfe62d6c80f0498cd7e76863e2f28e/packages/styled/types/base.d.ts

patsissons · 2019-11-14T22:53:19Z

it's possible the naming is just confusing. Themeless just means that the Theme type is empty and the field is then removed from the downstream types.

Andarist · 2019-11-14T23:11:51Z

I think this still interacts a little bit different with Theme. On next branch there are no conditional tests against Theme generic - it just gets passed along into appropriate types as argument.

Looking also for example at this:

emotion/packages/styled/types/base.d.ts

Lines 134 to 141 in ad77ed2

    
           <Tag extends keyof JSX.IntrinsicElements>( 
        
             tag: Tag, 
        
             options?: StyledOptions<JSX.IntrinsicElements[Tag]> 
        
           ): CreateStyledComponent< 
        
             { theme?: Theme }, 
        
             JSX.IntrinsicElements[Tag], 
        
             { theme: Theme } 
        
           >

and in Theme shows up twice - one is optional (passed as ComponentProps) and one is required (passed as StyleProps). I guess this might be the difference linked to my first statement in this comment - you don't allow at the moment a theme prop which overrides context theme.

patsissons · 2019-11-14T23:33:36Z

The conditional types were meant as an improvement to these types. i can completely remove the theme field from type system visibility (for style props) if it provides no value. I did have a mistake in my earlier types that is now corrected to allow overriding theme on styled components regardless of whether a contextual theme is present.

Andarist · 2019-11-15T09:03:32Z

I would prefer to keep those types structured in the very same way as @emotion/styled types are structured - because quite frankly we don't personally use @emotion/native. The main focus of emotion is web and changes are made primarily for related packages. Having those in sync would at least make it a lot easier to backport changes from @emotion/styled to this package.

I'm not yet entirely sure, but it's also very likely that Theme will get reworked quite a bit (but not focus on this as part of this PR - unless you want to wait till we merge changes to @emotion/styled). We plan to leverage interface merging so people can just augment built-in type with their typed Theme. This should simplify things quite a bit - the work is being done as part fo this PR #1609 if you want to take a look.

patsissons · 2019-11-15T19:52:35Z

ok, i reduced the types to just be analogies to the default styled types. We can't really share any types because of the react native stylesheet type (not without altering the styled types at least).

Andarist · 2019-11-16T09:02:27Z

We can't really share any types because of the react native stylesheet type (not without altering the styled types at least).

That's fine - sharing probably would make them more complex than they need to be.

Thanks for doing this! I'll probably merge this in over the weekend or shortly after that.

patsissons · 2019-11-19T00:42:14Z

Shall I move this out of draft?

Andarist · 2019-11-19T22:51:10Z

@patsissons yeah, you can do that. I'm in the middle of aligning those types just a little bit more to what we have in @emotion/styled now. (I'm in example including support for shouldForwardProp)

patsissons · 2019-11-20T03:23:01Z

sure thing, let me know if you want me to make any other changes.

Andarist · 2019-11-23T19:40:44Z

I've pushed out my changes - the work is not 100% done yet though. I would highly appreciate your review on this one.

From what I understand @emotion/native accepts a different set of CSS properties than "web" version and thus it doesn't quite make sense to reuse types from @emotion/serialize because they assume web. I've created copies of those which I have needed to make this work here.

Unfortunately, types which I have used to create ObjectInterpolation conflict with each other, so the created type is invalid:

emotion/packages/native/types/base.d.ts

Lines 14 to 18 in c3d48f9

    
           // @ts-ignore 
        
           export interface ObjectInterpolation 
        
             extends RN.ViewStyle, 
        
               RN.TextStyle, 
        
               RN.ImageStyle {}

I'm not sure what's the best way to work around this here. Any ideas?

Other than that tests need some work - I haven't really touched them, so they are using a type that doesn't exist anymore. Would you be willing to work on making those tests work?

patsissons · 2019-11-23T23:40:01Z

packages/native/types/base.d.ts

+type ReactNative = typeof RN
+
+export type ReactNativeStyle = ReturnType<ReactNative['StyleSheet']['flatten']>
+export type ObjectInterpolation = RN.ViewStyle | RN.TextStyle | RN.ImageStyle


this type union should be fine instead of the polymorphic version that was being used previously.

Seems like it's hard for us to create strict typings for those if we want to have types similar to those for the web version. From what I understand this will indeed work somewhat OK, but will allow incorrect object styles to be set on a particular component (in example it will allow text styles to be used on a view component). To keep things simple, I would say it's OK. WDYT?

I'll take another pass to see if I can improve it, but I think that may not be avoidable

patsissons · 2019-11-23T23:40:29Z

packages/native/types/base.d.ts

+  shouldForwardProp?(propName: PropertyKey): boolean
+}
+
+export interface StyledWithComponent<ComponentProps extends {}> {


open to a better name for this

patsissons · 2019-11-23T23:43:01Z

packages/native/types/base.d.ts

+    component: Component,
+    options?: StyledOptions<ComponentPropsWithoutRef<Component>>
+  ): CreateStyledComponent<
+    ComponentPropsWithoutRef<Component> & { theme?: Theme },


i wasn't sure why the custom component variant is using an intersection for component props rather than specific props like the react native components are below.

cc @JakeGinnivan, that was based on the types for @emotion/styled

Andarist · 2019-11-24T12:06:13Z

I see you have restructured typings a little bit. I'm not saying that those changes are bad, most of them are probably good - but also many of them could be considered rather cosmetic. I've intentionally written them like I've written them, so they could mirror what we have in for a web version.

Truth to be told @emotion/native has been created by a community member, we've merged it in and all - as it works. We don't have time to focus on it though in the same way as we focus on the web version. That's why I think it's crucial for us to have those types as close as possible to each other so we can sync them in the future. If we diverge them now it will just be way harder for us to maintain this as we are not using @emotion/native in any projects on our own.

If you believe that some changes you have made are particularly good we could evaluate if they could be backported to @emotion/styled as well - that could be done in followup PRs though.

patsissons · 2019-11-24T15:37:09Z

Most of the cosmetic/name changes were to help me trace how the types were being expanded so I could ensure all types were being forwarded through properly. Removal of the PropsOf helper and changing Tag to Component is because native has no concept of jsx tags, so it's specific to react native. If there were some other changes you would like to discuss I'm happy to explain any decisions and revert if that's desired. I should also mention that I made a number of changes to resolve linter errors, but ended up using a more appropriate tsconfig instead, so some of those changes could probably be reverted without any issue now. I'll try and comment on some of the changes now.

patsissons

hopefully these notes clear up some of the changes i made earlier. I'm going to rebase shortly to 🔥 the merge commit and cleanup the commit history.

packages/native/types/base.d.ts

patsissons · 2019-11-24T18:29:18Z

packages/native/types/base.d.ts


 export interface FunctionInterpolation<MergedProps> {
  (mergedProps: MergedProps): Interpolation<MergedProps>
 }

-export type Interpolation<MergedProps = undefined> =
+export type Interpolation<MergedProps = unknown> =


unknown is a more descriptive default for these MergedProps than undefined

packages/native/types/base.d.ts

patsissons · 2019-11-24T18:33:18Z

packages/native/types/base.d.ts

+export interface StyledWithComponent<ComponentProps extends {}> {
+  withComponent<
+    Component extends ComponentType<ComponentPropsWithoutRef<Component>>
+  >(
+    component: Component
+  ): StyledComponent<ComponentProps & ComponentPropsWithoutRef<Component>>
+  withComponent<ComponentName extends ReactNativeComponentNames>(
+    component: ReactNativeComponents[ComponentName]
+  ): StyledComponent<ComponentProps, ReactNativeComponentProps<ComponentName>>
+}


these withComponent signatures were pulled out into their own interface so that we can use a type union below for StyledComponent.

wouldnt we be able to do the same without pulling out into its own interface?

I think you're right, we don't need this to be its own interface any more (i think it was useful before when i needed to use a union).

patsissons · 2019-11-24T18:39:37Z

packages/native/types/base.d.ts

-  ): StyledComponent<ComponentProps, ReactNativeElements[Tag]>
-}
+> = ComponentType<ComponentProps & SpecificComponentProps> &
+  StyledWithComponent<ComponentProps>


It doesn't make much sense to type StyledComponent as an extension to a functional component. Technically it should be typed as a ForwardRefExoticComponent (and more generally, NamedExoticComponent). This might be a more appropriate choice (i'll give it a try in a moment).

NamedExoticComponent works appropriately here so i've gone with that choice.

this makes sense, cc @JakeGinnivan - shouldn't we do the same for web types?

packages/native/types/base.d.ts

packages/native/types/tslint.json

patsissons

@Andarist the interpolation types are now working with inference, I think that's about as far as I can go with the typings. LMK how you want to proceed from here (feel free to commit on top and adjust names or squash the branch at this point)

packages/native/types/base.d.ts

patsissons · 2019-11-24T20:28:22Z

packages/native/types/base.d.ts

+export type ReactNativeStyleType<Props> = Props extends {
+  style?: RN.StyleProp<infer StyleType>
+}
+  ? StyleType
+  : ReactNativeStyle


this construct lets us extract the specific shape of a react-native style prop, and support custom components too.

patsissons · 2019-11-24T20:28:55Z

packages/native/types/base.d.ts

-  ): StyledComponent<ComponentProps, ReactNativeElements[Tag]>
-}
+> = ComponentType<ComponentProps & SpecificComponentProps> &
+  StyledWithComponent<ComponentProps>


NamedExoticComponent works appropriately here so i've gone with that choice.

patsissons · 2019-11-24T20:30:35Z

packages/native/types/base.d.ts

+  ComponentProps extends {},
+  SpecificComponentProps extends {} = {},
+  StyleProps extends {} = {},
+  StyleType = ReactNativeStyle


I found that the StyleType, while properly inferred, was allowing an ImageStyle to be used as a TextStyle but not the other way around. Feels like a bug in the TypeScript inference logic because i couldn't see a relation between the two, but i couldn't get a better answer as to why (i left some comments in the test file).

type True = RN.ImageStyle extends RN.TextStyle ? true : false type False = RN.TextStyle extends RN.ImageStyle ? true : false

Seems like ImageStyle is a subtype of TextStyle

haha interesting, i'm not going to dig further since it's already difficult to understand why that may be.

Similarly on web input is a subtype of div. It's not like those types form such hierarchy, but rather that div is so basic, that it has no special props associated with it and everything that can be used on it can be used on input as well (+ input has some specific props for it). And because TS type system is structural rather than nominal this forms implicit supertype-subtype relationship.

patsissons · 2019-11-24T20:31:30Z

packages/native/types/base.d.ts

+    ComponentPropsWithoutRef<Component> & { theme?: Theme },
+    {},
+    { theme: Theme },
+    ReactNativeStyleType<ComponentPropsWithoutRef<Component>>


as long as the custom component defines a style?: StyleProp<T>, the T will get inferred and applied to the template interpolation types.

packages/native/types/index.d.ts

patsissons · 2019-11-24T20:33:43Z

packages/native/types/index.d.ts

+    { theme?: Theme },
+    ReactNativeComponentProps<ComponentName>,
+    { theme: Theme },
+    ReactNativeStyleType<ReactNativeComponentProps<ComponentName>>


all styled react-native components should now be strongly typed against their specific style prop shape.

patsissons · 2019-11-24T23:35:21Z

I ran into some composition issues testing out these types with a more complicated setup. The react-native styles sometimes override the definitions of certain style fields which can cause grief if you want to share some common styles and compose them downstream. I updated the css function signature to support optionally explicit typing to solve this, as well as implicit typing if the function is used with simple style objects (though these won't be type inferred at definition time). This appears to be working properly in my project, so hopefully it's a worthwhile fix (i added some new test code to cover these types of scenarios).

Andarist · 2019-12-03T08:46:53Z

@patsissons sorry that I haven't yet reviewed this, I hope to do it this week

Andarist · 2019-12-15T19:36:38Z

packages/native/types/index.d.ts

+
+export function css<StyleType extends ReactNativeStyle = ReactNativeStyle>(
+  template: TemplateStringsArray,
+  ...args: Array<Interpolation>


I'm not sure if this should use Interpolation. For instance, it doesn't make sense for css to accept functions - for the web we've split this into two similar, but separate, Interpolation and CSSInterpolation. I feel like the same should be done here.

makes sense to me, i can add those suggestions in pretty quickly

Andarist · 2019-12-15T19:38:19Z

@patsissons sorry that I haven't reviewed this sooner but had plenty on my plate. Let me know if you are still interested in working on this one (not much left to do).

patsissons · 2019-12-15T20:16:44Z

@patsissons sorry that I haven't reviewed this sooner but had plenty on my plate. Let me know if you are still interested in working on this one (not much left to do).

No worries, i'm in the same boat. I've addressed your review suggestions and things looks reasonable to me right now. My mind isn't as deep into these types as it was a few weeks ago so maybe spot check the changes i made. Otherwise i think i'm satisfied with these types for an MVP release, i'll try and pull them into a project later today to give them a broader test.

Andarist · 2019-12-15T20:50:44Z

packages/native/types/base.d.ts

+  StyleType extends ReactNativeStyle = ReactNativeStyle
+> extends Array<CSSInterpolation<MergedProps, StyleType>> {}
+
+export type CSSInterpolation<


I don't think that MergedProps props are actually used anyhow inside CSSInterpolation, it only seems to be passed around, but not utilized. This is good though - with css you don't get access to any props at all. This means though that we can just drop this typeparam here.

yup, absolutely. a bit of blind refactoring lands you with useless type params 😄

patsissons · 2019-12-15T23:36:30Z

@Andarist are we good to ship? 🚢

* adding types for react native * fixing styled component types * removing condition type inference for existence of a theme * Move @emotion/native TS types closer to what we have for @emotion/styled * some minor adjustments to the types * updating interpolation types to support inferred style type * addressing some deficiencies * Use Theme interface from @emotion/core in @emotion/native * adding review suggestions * Add changeset

patsissons force-pushed the next-native-types branch from a662cbc to 4f8a984 Compare November 14, 2019 20:14

patsissons force-pushed the next-native-types branch from 1a5c6b2 to 77de716 Compare November 14, 2019 23:32

Andarist mentioned this pull request Nov 18, 2019

Added basic support for accepting custom shouldForwardProp option in React Native related packages. #1642

Merged

patsissons marked this pull request as ready for review November 20, 2019 03:23

patsissons commented Nov 23, 2019

View reviewed changes

patsissons force-pushed the next-native-types branch from e9a525d to 1343b88 Compare November 24, 2019 00:06

patsissons force-pushed the next-native-types branch 2 times, most recently from 1781c39 to 755ca6c Compare November 24, 2019 18:58

patsissons commented Nov 24, 2019

View reviewed changes

Pat Sissons added 2 commits November 24, 2019 12:23

adding types for react native

84d791e

fixing styled component types

832cd09

Pat Sissons and others added 3 commits November 24, 2019 12:23

removing condition type inference for existence of a theme

e93ffd3

Move @emotion/native TS types closer to what we have for @emotion/styled

1792f92

some minor adjustments to the types

2afd7ac

patsissons force-pushed the next-native-types branch from 529c6ea to 7a4a433 Compare November 24, 2019 20:24

updating interpolation types to support inferred style type

c2f6f6f

patsissons force-pushed the next-native-types branch from 7a4a433 to c2f6f6f Compare November 24, 2019 20:33

patsissons commented Nov 24, 2019

View reviewed changes

addressing some deficiencies

b7bea71

patsissons force-pushed the next-native-types branch from 329f542 to b7bea71 Compare November 24, 2019 23:46

Andarist added 2 commits December 15, 2019 06:52

Merge branch 'next' into next-native-types

29c63b3

Use Theme interface from @emotion/core in @emotion/native

0ef50a5

Andarist reviewed Dec 15, 2019

View reviewed changes

adding review suggestions

cf8cb89

patsissons force-pushed the next-native-types branch from e8a542b to cf8cb89 Compare December 15, 2019 20:55

Andarist approved these changes Dec 15, 2019

View reviewed changes

Add changeset

2f08db6

Andarist merged commit 456be9a into emotion-js:next Dec 16, 2019

This was referenced Dec 16, 2019

Typescript definitions for @emotion/native #839

Closed

React Native improvements: correct components and TypeScript types #1176

Closed

github-actions bot mentioned this pull request Nov 10, 2020

Version Packages #2046

Merged

adding types for @emotion/native #1634

adding types for @emotion/native #1634

Conversation

patsissons commented Nov 14, 2019

changeset-bot bot commented Nov 14, 2019 • edited Loading

🦋 Changeset is good to go

codesandbox-ci bot commented Nov 14, 2019 • edited Loading

Andarist commented Nov 14, 2019

patsissons commented Nov 14, 2019

Andarist commented Nov 14, 2019

patsissons commented Nov 14, 2019

Andarist commented Nov 14, 2019

patsissons commented Nov 14, 2019

Andarist commented Nov 15, 2019

patsissons commented Nov 15, 2019

Andarist commented Nov 16, 2019

patsissons commented Nov 19, 2019

Andarist commented Nov 19, 2019

patsissons commented Nov 20, 2019

Andarist commented Nov 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Nov 24, 2019

patsissons commented Nov 24, 2019 • edited Loading

patsissons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patsissons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patsissons commented Nov 24, 2019 • edited Loading

Andarist commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Dec 15, 2019

patsissons commented Dec 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patsissons commented Dec 15, 2019

changeset-bot bot commented Nov 14, 2019 •

edited

Loading

codesandbox-ci bot commented Nov 14, 2019 •

edited

Loading

patsissons commented Nov 24, 2019 •

edited

Loading

patsissons commented Nov 24, 2019 •

edited

Loading