DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

Typescript: improve enum types and setMargin type #1233

Closed
bbohlender wants to merge 1 commits from main into main
pull from: main
merge into: DaddyFrosty:main
Conversation 6 Commits 1 Files Changed 1 +82911 -186602
bbohlender commented 2023-02-26 04:15:37 -08:00 (Migrated from github.com)
Copy Link

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.

Additionally, this PR fixes the incorrect typing of setMargin(edge: Edge, margin: number) by extending the type to margin: number | string

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. Additionally, this PR fixes the incorrect typing of `setMargin(edge: Edge, margin: number)` by extending the type to `margin: number | string`
NickGerleman (Migrated from github.com) requested changes 2023-02-27 05:41:32 -08:00
NickGerleman (Migrated from github.com) left a comment
Copy Link

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.

> 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.
bbohlender commented 2023-02-27 11:22:28 -08:00 (Migrated from github.com)
Copy Link

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 0 | 1 | 2).
Using the enums from typescript would make sense here since they would be present when hovering/autocompleting and would look like: node.setAlignItems(Align.ALIGN_AUTO). The constants could still be exported via export ALIGN_AUTO = Align.ALIGN_AUTO to not break anything.

Nevertheless, I reverted the changes, so this PR only includes the type fix for setMargin :)

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 `0 | 1 | 2`). Using the `enum`s from typescript would make sense here since they would be present when hovering/autocompleting and would look like: `node.setAlignItems(Align.ALIGN_AUTO)`. The constants could still be exported via `export ALIGN_AUTO = Align.ALIGN_AUTO` to not break anything. Nevertheless, I reverted the changes, so this PR only includes the type fix for `setMargin` :)
NickGerleman commented 2023-02-28 13:52:00 -08:00 (Migrated from github.com)
Copy Link

Using the enums from typescript would make sense here since they would be present when hovering/autocompleting and would look like: node.setAlignItems(Align.ALIGN_AUTO). The constants could still be exported via export ALIGN_AUTO = Align.ALIGN_AUTO to not break anything.

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 const enum has well defined runtime semantics that could be matched (and usable from vanilla JS):

  1. https://babeljs.io/docs/babel-plugin-transform-typescript#optimizeconstenums
  2. https://www.typescriptlang.org/docs/handbook/enums.html#const-enums
> Using the enums from typescript would make sense here since they would be present when hovering/autocompleting and would look like: node.setAlignItems(Align.ALIGN_AUTO). The constants could still be exported via export ALIGN_AUTO = Align.ALIGN_AUTO to not break anything. 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 `const enum` has well defined runtime semantics that could be matched (and usable from vanilla JS): 1. https://babeljs.io/docs/babel-plugin-transform-typescript#optimizeconstenums 2. https://www.typescriptlang.org/docs/handbook/enums.html#const-enums
NickGerleman (Migrated from github.com) approved these changes 2023-02-28 13:52:10 -08:00
javascript/src_js/wrapAsm.d.ts
@@ -0,0 +136,4 @@
setJustifyContent(justifyContent: Justify): void,
setGap(gutter: Gutter, gapLength: number): Value,
setMargin(edge: Edge, margin: number | string): void,
setMarginAuto(edge: Edge): void,
NickGerleman (Migrated from github.com) commented 2023-02-28 13:35:56 -08:00
Copy Link

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 | 'auto'  | `${number}%`): void,
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). ```suggestion setMargin(edge: Edge, margin: number | 'auto' | `${number}%`): void, ```
facebook-github-bot commented 2023-03-01 05:46:50 -08:00 (Migrated from github.com)
Copy Link

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff [on Phabricator](https://www.internalfb.com/diff/D43695520).
facebook-github-bot commented 2023-03-01 07:44:44 -08:00 (Migrated from github.com)
Copy Link

@NickGerleman merged this pull request in facebook/yoga@c09405d58c.

@NickGerleman merged this pull request in facebook/yoga@c09405d58c624fd26d5e0a7ed98bd10c5cc4c05b.

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
NickGerleman
Labels
Clear labels
Algorithm
Build and Infrastructure
C#
C++ Enhancement
CLA Signed
Discussion
Documentation
Feature Request
Java
JavaScript
Merged
Question
Reverted
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
YogaKit
YogaLayout ViewGroup
bug
dependencies
Pull requests that update a dependency file
duplicate
enhancement
fb-exported
fh:direct-merge-enabled
invalid
ruby
Pull requests that update Ruby code
wontfix
No Label
CLA Signed
Merged
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
DaddyFrosty
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#1233
No description provided.
Delete Branch "main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?