fix(css-properties): remove incorrect React.ReactChild from Lossy Union #372

luke-john · 2018-01-08T02:32:11Z

What:

Remove React.ReactChild from css properties Lossy Unions

This was originally added to fix a problem where built in glamorous components had been typed to accept the css properties union which included the lossy version.

By updating that component to instead use the css CompleteSingle interface the React.ReactChild is able to be removed.

Checklist:

Documentation N/A
Tests
Ready to be merged

codecov-io · 2018-01-08T02:35:14Z

Codecov Report

Merging #372 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #372   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         182    182           
  Branches       52     52           
=====================================
  Hits          182    182

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b843c27...8738c76. Read the comment docs.

denis-sokolov · 2018-01-08T07:26:01Z

This clearly looks like a better solution. However, with TypeScript 2.6.1 your PR fails to typecheck my codebase.

<Div height={18}>
  <MyComponent />
</Div>

error TS2322: Type '{ height: 18; children: Element; }' is not assignable to type 'IntrinsicAttributes & CSSProperties & ExtraGlamorousProps & HTMLProps<HTMLDivElement> & { childre...'.
  Type '{ height: 18; children: Element; }' is not assignable to type 'CSSProperties'.
    Property 'children' is incompatible with index signature.
      Type 'Element' is not assignable to type 'string | number | SingleOrArray<CSSPropertiesCompleteSingle, "alignContent" | "alignItems" | "ali...'.
        Type 'Element' is not assignable to type '(string | number | {} | ("inherit" | "initial" | "unset" | "flex-start" | "flex-end" | "center" |...'.

I don’t understand why, since the test cases clearly include this. :-(

luke-john · 2018-01-08T08:11:31Z

🤔 Are you able to share a version of MyComponent that is failing? I'm unable reproduce the issue locally (using 2.6.1 and 2.6.2).

I've added additional cases to test component signatures being accepted as children to this pr. https://github.com/luke-john/glamorous/blob/4a1598ca1a97d72c02ab54ecaccea3a1b9d8133c/test/glamorous.test.tsx#L568-L585

luke-john · 2018-01-08T08:15:47Z

actually are you sure you're using this version? I'm a little bit suspicious of that error containing CSSProperties.

error TS2322: Type '{ height: 18; children: Element; }' is not assignable to type 'IntrinsicAttributes & CSSProperties & ExtraGlamorousProps & HTMLProps<HTMLDivElement> & { childre...'.

denis-sokolov · 2018-01-08T20:58:11Z

Hm, strange. I will test more thoroughly in the next day or two.

Ailrun · 2018-01-08T21:32:14Z

@luke-john, @denis-sokolov is right.
Since by

{
  [propertyName: string]: T
}

T is intersected with every properties including children, T should include the types for children or ref, onClick ...

Ailrun · 2018-01-08T21:33:03Z

typings/css-properties.d.ts

@@ -1926,7 +1926,6 @@ export interface CSSPropertiesLossy {
    | undefined
    | Array<CSSPropertiesComplete[keyof CSSPropertiesComplete]>
    | CSSPropertiesLossy
-    | React.ReactChild


This will cause the same problem of #353

This is the minimal PoC for common problem.
https://goo.gl/kjc8XD

IMO, to remove this extra union (as we want), we need to solve #353 first.

luke-john · 2018-01-09T03:11:34Z

Ahh right, cheers for that @Ailrun, I've recently left my last workplace so don't have any large codebases to test prs on at the moment.

Something is clearly going wrong here as the type for glamorous.Div is not matching the type for the named export Div.

luke-john · 2018-01-09T03:17:19Z

🤦‍♂️ Ahh I forgot to read the important comment at the top of that file

// The file `./named-built-in-glamorous-components.d.ts` is based off this file
// and should get any updates this file does.

will update shortly. thanks

Ailrun · 2018-01-09T05:17:16Z

typings/built-in-glamorous-components.d.ts

  >
  Wbr: React.StatelessComponent<
-    CSSProperties & ExtraGlamorousProps & React.HTMLProps<HTMLElement>
+    CSSPropertiesCompleteSingle &


So being unable to use ':active' or other pseudo properties works as intended?
I mean,

const props = { ':active': { color: 'purple' } }; <glamorous.A {...props} />

Is invalid?
I beg your understanding for my ignorance of whole project structure itself. (I just skim whole codes after I became a collaborator, because of lack of time...)

hmm, I hadn't really thought about spreading the css props on.

🤔 That is valid, but as I understood it the purpose of that api was to make it possible to simple add css properties like follows;

<glamorous.A color="purple" />

And for the pattern you're describing do something like follows

const cssProps = { ':active': { color: 'purple' } }; <glamorous.A css={props} />

I'm leaning towards making the types simpler here to reflect the intended use. As otherwise it makes the types less useful elsewhere.

Really appreciate your feedback and taking the time to go over these prs and even skimming the code 💖.

We could add CSSPropertiesPseudo to the Intersection which would allow the case you've raised to work. But it would still fall down on things like

const props = { '> span': { color: 'purple' } }; <glamorous.A {...props}>hello <span>world</span></glamorous.A>

Thank you for your kind answer!
I think I fully get the point what you want to say, and I agree with it to some distance. I will approve this PR.

I've added test cases for the pseudo props as part of this pr as well, cheers.

https://github.com/luke-john/glamorous/blob/a821469f2cad3c8511bbdfc12d2c807d6cc4d08f/test/glamorous.test.tsx#L611-L621

This was originally added to fix a problem where built in glamorous components had been typed to accept the css properties union which included the lossy version. By updating that component to instead use the css CompleteSingle interface the React.ReactChild is able to be removed.

Ailrun · 2018-01-10T21:39:28Z

@luke-john Following is just a question. If you give me an answer, I would greatly appreciate it.

Is there any reason for dismissing the review?

denis-sokolov · 2018-01-10T21:50:26Z

My speculation would be Luke wanted to express a need for you to review it afresh after the latest changes, @Ailrun.

I promised to test this again until today, @luke-john, but my schedule can’t allow it, apologies. I won’t be able to reproduce it until the weekend.

luke-john · 2018-01-10T23:01:56Z

@Ailrun I pushed the CSSPropertiesPseudo and test case additions for #372 (comment) after your review.

And GitHub automatically dismisses reviews when changes are made to a pull request.

Ailrun · 2018-01-10T23:13:23Z

@luke-john Thank you for explanation! I didn't know there's a case that github automatically dismisses a review.
Anyway, from my point of view, the PR looks OK.

Ailrun · 2018-01-10T23:15:59Z

And also thank you for @denis-sokolov to try to explain what I ask! I really appreciate you all!

denis-sokolov · 2018-01-11T07:46:00Z

GitHub does not always automatically dismiss a review, it’s a per-repository setting. Some people want to make sure that the reviewers have seen the absolutely latest version of the PR, and some are okay with relying on a more lax rule of common sense.

denis-sokolov · 2018-01-21T14:12:28Z

I’m confused by my tests. Minimal case that triggers the error with this branch as the dependency is this:

<Div>
  <Div />
</Div>

Obviously it is supposed to be covered in tests, but I was pretty thorough with the tests. Building glamorous at 8a0216f makes my tsc succeed, then I switch glamorous to 7912617, and tsc starts loudly failing.

This happens with tsc 2.6.1 and 2.6.2, confirmed the flow with both.

Here is the error:

index.tsx(90,7): error TS2322: Type '{ children: Element; }' is not assignable to type 'IntrinsicAttributes & CSSProperties & ExtraGlamorousProps & HTMLProps<HTMLDivElement> & { childre...'.
  Type '{ children: Element; }' is not assignable to type 'CSSProperties'.
    Property 'children' is incompatible with index signature.
      Type 'Element' is not assignable to type 'string | number | SingleOrArray<CSSPropertiesCompleteSingle, "alignContent" | "alignItems" | "ali...'.
        Type 'Element' is not assignable to type '(string | number | {} | ("inherit" | "initial" | "unset" | "flex-start" | "flex-end" | "center" |...'.```

GirlBossRush · 2018-02-09T00:15:13Z

@Ailrun,

This looks awesome! Is there an ETA to release?

Ailrun · 2018-02-11T10:36:07Z

@TeffenEllis Sorry for late response. @kentcdodds could you answer to @TeffenEllis ?

kentcdodds · 2018-02-11T14:37:26Z

The releases for this project are automatic. So it should be released as soon as this is merged.

Ailrun · 2018-02-11T15:26:45Z

@kentcdodds Oh, I mean... when will this merged?

kentcdodds · 2018-02-11T17:51:00Z

I keep out of typescript related stuff unless specifically asked. Do you all my help here?

Feel free to merge typescript pull requests yourself. Just make sure to follow the commit message convention when doing squash and merge so the right version is released.

Ailrun · 2018-02-11T18:58:41Z

@kentcdodds I got
this message so I thought I can't merge this.
Am I wrong? Can I merge this?
If that's the case, I will merge this (and after, other TS related PRs).

kentcdodds · 2018-02-12T17:11:21Z

It's because the PR was not updated with master, so I just clicked the "Update branch" button and once the status checks are done you should be able to merge it.

Just make sure to follow the commit message conventions when you squash and merge, otherwise a new version will not be released.

Cheers! And thanks!

Ailrun · 2018-02-12T17:41:57Z

@luke-john Please update your snapshots.
It seems this snapshot changes related with microsoft/TypeScript#20705.

luke-john · 2018-02-16T09:04:11Z

Have just updated the tests, I'm unsure about merging this though as we never worked out why this was causing additional failures #372 (comment). I've tried to reproduce without luck, however have added a test case for this which does not appear to fail.

Also sorry for the huge delay here, I've just relocated to London from Australia and my life has been a bit hectic lately.

luke-john · 2018-02-16T09:04:31Z

thanks for following this up @Ailrun

Ailrun · 2018-02-16T16:44:59Z

@luke-john it looks like @denis-sokolov miss something (or we miss some updates) since error message includes CssProperties which we just removed in this PR. I will also retry to reproduce, and if it's failed, IMO, we should merge this first and make an issue with needs investigation label.

P.S. I cannot reproduce it.

Ailrun · 2018-02-16T18:38:47Z

@kentcdodds I still cannot merge this.

There're no buttons I can click :(.

kentcdodds · 2018-02-16T19:21:54Z

Oh, that's because you're the one who approved it. Need to have someone else merge it. I guess that can be me! :)

luke-john added the TypeScript label Jan 8, 2018

luke-john force-pushed the fix/built-in-glamorous-components_properties branch from 7b5ad84 to 7912617 Compare January 8, 2018 02:38

luke-john mentioned this pull request Jan 8, 2018

fix(typings): allow to have primitives as children for built-in #369

Closed

4 tasks

luke-john force-pushed the fix/built-in-glamorous-components_properties branch from a10f44d to 4a1598c Compare January 8, 2018 08:05

Ailrun suggested changes Jan 8, 2018

View reviewed changes

luke-john force-pushed the fix/built-in-glamorous-components_properties branch from 4a1598c to 5716958 Compare January 9, 2018 03:37

luke-john mentioned this pull request Jan 9, 2018

Typescript fails on Div and Button elements, if you give the onClick attribute a function as argument #353

Closed

Ailrun reviewed Jan 9, 2018

View reviewed changes

Ailrun previously approved these changes Jan 9, 2018

View reviewed changes

luke-john added 2 commits January 9, 2018 14:32

chore(typescript): add more test cases for built in components

a821469

luke-john dismissed Ailrun’s stale review via a821469 January 9, 2018 06:32

luke-john force-pushed the fix/built-in-glamorous-components_properties branch from 5716958 to a821469 Compare January 9, 2018 06:32

Ailrun previously approved these changes Jan 10, 2018

View reviewed changes

Ailrun mentioned this pull request Feb 11, 2018

typescript: built-in components don't accept Components as children #304

Closed

Ailrun mentioned this pull request Feb 11, 2018

Update failing Typescript failure snapshot #382

Closed

3 tasks

Ailrun force-pushed the fix/built-in-glamorous-components_properties branch from cdc9dbc to a821469 Compare February 11, 2018 19:05

Merge branch 'master' into fix/built-in-glamorous-components_properties

5942dad

chore(typescript): update snapshot for new typescript version

daab1fd

luke-john dismissed Ailrun’s stale review via daab1fd February 16, 2018 08:54

chore(typescript): add reported failing Div test case

8738c76

Ailrun approved these changes Feb 16, 2018

View reviewed changes

Ailrun mentioned this pull request Feb 16, 2018

[TS] Fails with children after #372 #384

Closed

kentcdodds merged commit a2dcedf into paypal:master Feb 16, 2018

This was referenced Feb 16, 2018

Update 25 dependencies from npm storybookjs/storybook#3003

Merged

Update glamorous in / from 4.11.2 to 4.11.6 dailybruin/sources#123

Closed

This was referenced Mar 16, 2018

Update 10 dependencies from npm uclaradio/panel#23

Closed

Update 10 dependencies from npm uclaradio/panel#25

Closed

Update 10 dependencies from npm uclaradio/panel#26

Closed

dependencies bot mentioned this pull request Apr 1, 2018

Update 14 dependencies from npm uclaradio/panel#27

Closed

fix(css-properties): remove incorrect React.ReactChild from Lossy Union #372

fix(css-properties): remove incorrect React.ReactChild from Lossy Union #372

Conversation

luke-john commented Jan 8, 2018 • edited by Ailrun Loading

codecov-io commented Jan 8, 2018 • edited Loading

Codecov Report

denis-sokolov commented Jan 8, 2018

luke-john commented Jan 8, 2018

luke-john commented Jan 8, 2018

denis-sokolov commented Jan 8, 2018

Ailrun commented Jan 8, 2018 • edited Loading

Ailrun Jan 8, 2018

Choose a reason for hiding this comment

Ailrun Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Ailrun Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

luke-john commented Jan 9, 2018 • edited Loading

luke-john commented Jan 9, 2018

Ailrun Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

luke-john Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

luke-john Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Ailrun Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

luke-john Jan 9, 2018

Choose a reason for hiding this comment

Ailrun commented Jan 10, 2018

denis-sokolov commented Jan 10, 2018

luke-john commented Jan 10, 2018

Ailrun commented Jan 10, 2018 • edited Loading

Ailrun commented Jan 10, 2018

denis-sokolov commented Jan 11, 2018

denis-sokolov commented Jan 21, 2018

GirlBossRush commented Feb 9, 2018

Ailrun commented Feb 11, 2018

kentcdodds commented Feb 11, 2018

Ailrun commented Feb 11, 2018

kentcdodds commented Feb 11, 2018

Ailrun commented Feb 11, 2018 • edited Loading

kentcdodds commented Feb 12, 2018

Ailrun commented Feb 12, 2018 • edited Loading

luke-john commented Feb 16, 2018

luke-john commented Feb 16, 2018

Ailrun commented Feb 16, 2018 • edited Loading

Ailrun commented Feb 16, 2018

kentcdodds commented Feb 16, 2018

luke-john commented Jan 8, 2018 •

edited by Ailrun

Loading

codecov-io commented Jan 8, 2018 •

edited

Loading

Ailrun commented Jan 8, 2018 •

edited

Loading

Ailrun Jan 8, 2018 •

edited

Loading

Ailrun Jan 8, 2018 •

edited

Loading

luke-john commented Jan 9, 2018 •

edited

Loading

Ailrun Jan 9, 2018 •

edited

Loading

luke-john Jan 9, 2018 •

edited

Loading

luke-john Jan 9, 2018 •

edited

Loading

Ailrun Jan 9, 2018 •

edited

Loading

Ailrun commented Jan 10, 2018 •

edited

Loading

Ailrun commented Feb 11, 2018 •

edited

Loading

Ailrun commented Feb 12, 2018 •

edited

Loading

Ailrun commented Feb 16, 2018 •

edited

Loading