Typescript: improve enum types and setMargin type #1233
Conversation
There was a problem hiding this comment.
Currently, it is impossible to write node.setPositionType(2) instead of node.setPositionType(POSITION_TYPE_ABSOLUTE). I understand that the idea is to force explicit usage of the enums. However, declaring type POSITION_TYPE_ABSOLUTE = 2 & ['POSITION_TYPE'] artificially limits use cases, where, for example, the state should be serialized typesafe.
This was an intentional design decision, so that the underlying data type is opaque/an implementation detail. It also avoids issues like enum types being exposed on hover as entities like 0 | 1 | 2, or being able to plug any enum into any method (it removes the type safety).
If you are wanting to serialize these enums externally, I would recommend adding something like a switch/case over whatever format you are using. I.e. do the mapping yourself from the number to the proper opaque type.
|
On the typescript version, I am currently at (4.9.5); typescript offers no options on hover/autocomplete (I wonder if this is preferred over offering Nevertheless, I reverted the changes, so this PR only includes the type fix for |
I think this is a good solution, with the caveat that the TypeScript enums are a runtime concept, and the underlying source is not currently TypeScript. I am not sure if it can be kosher to replicate the same structure in JS, then expose in typings as an enum, or if we really should be making the source TypeScript to do that. It seems like |
| @@ -135,7 +135,7 @@ export type Node = { | |||
| setHeightPercent(height: number): void, | |||
| setJustifyContent(justifyContent: Justify): void, | |||
| setGap(gutter: Gutter, gapLength: number): Value, | |||
| setMargin(edge: Edge, margin: number): void, | |||
| setMargin(edge: Edge, margin: number | string): void, | |||
There was a problem hiding this comment.
This matches the other types here (converted from previous flowtype typings), but we could also replace this + others with a template literal type for better safety (added in TS 4.1, which is the oldest supported by DefinitelyTyped).
| setMargin(edge: Edge, margin: number | string): void, | |
| setMargin(edge: Edge, margin: number | 'auto' | `${number}%`): void, |
|
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@NickGerleman merged this pull request in c09405d. |
to apply fix - facebook/yoga#1233
to apply fix - facebook/yoga#1233
Summary: - format types with `prettier` - apply suggestion from #1233 (comment) Pull Request resolved: #1236 Reviewed By: javache Differential Revision: D44052580 Pulled By: NickGerleman fbshipit-source-id: 0d9810da460cf4290e15308acdbb705c71f8d8a1
Currently, it is impossible to write
node.setPositionType(2)instead ofnode.setPositionType(POSITION_TYPE_ABSOLUTE). I understand that the idea is to force explicit usage of the enums. However, declaringtype POSITION_TYPE_ABSOLUTE = 2 & ['POSITION_TYPE']artificially limits use cases, where, for example, the state should be serialized typesafe.Additionally, this PR fixes the incorrect typing of
setMargin(edge: Edge, margin: number)by extending the type tomargin: number | string