Improve the Flow type generator

[keyz]

First of all, thank you for creating this library.

I made some changes to the Flow type generator:

1. Use @flow strict. This allows @flow strict projects to consume the type.
2. Use exact types ({| ... |}) by default. Now if you add an unknown property, Flow will warn. (See __tests__/__snapshots__/flow.ts.snap)
3. Use type spread (...) instead of intersection types (&). Intersection types are weird. We generally prefer using exact types and type spread. https://flow.org/en/docs/types/objects/#toc-exact-object-types
4. Lift all "fallbackable" types (e.g. string | string[]) to the top of the file. Unfortunately there's a bug in Flow (Fix object spreading, issues 6108, 7242, 5243, 7210, 7253, 6357, 5253, 4682, 6800, 981, 7048 and more facebook/flow#7298) related to type spread and union types. An easy way to fix it is to lift the duplicated union types to type aliases.
5. Created a fixture and a snapshot for expected errors.

(here's the error if we don't lift the types)

Error ----------------------------------------------------------------------------------------------- typecheck.js:23:12

Could not decide which case to select. Since case 1 [1] may work but if it doesn't case 2 [2] looks promising too. To
fix add a type annotation to inferred union of array element types (alternatively, provide an annotation to summarize
the array element type) [3].

   typecheck.js:23:12
     23|   opacity: [1],
                    ^^^ [3]

References:
   index.js.flow:1798:13
   1798|   opacity?: GlobalsNumber | Array<GlobalsNumber>,
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1]
   index.js.flow:2355:13
   2355|   opacity?: GlobalsNumber | Array<GlobalsNumber>,
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [2]


Error ----------------------------------------------------------------------------------------------- typecheck.js:25:15

Could not decide which case to select. Since case 1 [1] may work but if it doesn't case 2 [2] looks promising too. To
fix add a type annotation to inferred union of array element types (alternatively, provide an annotation to summarize
the array element type) [3].

   typecheck.js:25:15
     25|   fontWeight: ['normal'],
                       ^^^^^^^^^^ [3]

References:
   index.js.flow:1717:16
   1717|   fontWeight?: FontWeightProperty | Array<FontWeightProperty>,
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1]
   index.js.flow:2344:16
   2344|   fontWeight?: FontWeightProperty | Array<FontWeightProperty>,
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [2]



Found 2 errors

[keyz]

friendly ping @frenic

[frenic]

Waw, I love this! Thank you! ❤️

First of, I'm not using Flow. So I'm hoping someone else with more Flow knowledge could do a review too. Maybe @rtsao?

Is if there's a way to use missing vendor specific properties or custom properties concerning it's using exact types by default? How would you solve this?

const css: CSS.Properties<*> = {
  '--my-color': 'black',
};

This change should also be treated as a major change if I understand it correctly?

[TrySound]

@frenic If it's really required

const css: { ...CSS.Properties<*> } = {
  '--my-color': 'black',
};

[TrySound]

Also this is equal to just string and makes all this types useless for value validating.
https://github.com/frenic/csstype/blob/master/index.js.flow#L4012

export type AlignContentProperty = Globals | ContentDistribution | ContentPosition | "baseline" | "normal" | string;

[frenic]

Also this is equal to just string and makes all this types useless for value validating.
https://github.com/frenic/csstype/blob/master/index.js.flow#L4012

export type AlignContentProperty = Globals | ContentDistribution | ContentPosition | "baseline" | "normal" | string;

It's useless for TypeScript too and will most likely forever be useless for validation. But we're hoping that one day that tools like VSCode will provide autocompletion using these literals. There's an issue regarding this already. They are kept because there hasn't been any reason to remove them yet. No major performance benefits or anything like that since last time I checked. But the case could be different for Flow?

[keyz]

@frenic

Is if there's a way to use missing vendor specific properties or custom properties concerning it's using exact types by default? How would you solve this?

You can change it to an inexact type as @TrySound mentioned, but I think it'd be better to define the custom properties in userland. Something like:

type ColorVariables = {|
  '--my-color': string,
  '--my-color-2': string,
|};

function getSomeStyle(): {|...CSS.Properties<*>, ...$Shape<ColorVariables>|} {
  ...
}

The nice thing about $Shape<...> is that it marks the fields optional, but still keeps the type exact.

This change should also be treated as a major change if I understand it correctly?

Yeah, this would be a breaking change.

Also this is equal to just string and makes all this types useless for value validating

I actually didn't notice that string is included in the types! Yeah I think it'd be nice to drop them, but that sounds like a bigger change.

[frenic]

I actually didn't notice that string is included in the types! Yeah I think it'd be nice to drop them, but that sounds like a bigger change.

string is included where properties can have combined values like display: block flow;. Experiments with combining literals is in the pipeline to get rid of string on some properties. I would be nice to see some kind of performance test results with removed string literals in Flow before considering getting rid of them permanently. Here's a branch with removed unnecessary string literals and completely resolved data types if someone would like to help out.

[keyz]

Thanks @frenic. I think the potential gains of dropping the string is more on the type-safety side. I don't know that much about Flow internals but I guess having string should actually make it more performant (since there're fewer "branches" to check).

[frenic]

I did a quick check on a huge Typescript application using CSS-in-JS with csstype and there's not even a tiny performance difference when type checking between the latest release of csstype and the one with removed string literals. Typescript simplifies "foo" | "bar" | string early in the type checking process to just string so I guess that's why it doesn't affect performance. Hopefully it's the same for Flow. Let's keep that as is for now.

[frenic]

Thank you for this 👏 I'm merging this into separate branch as a part for v3.