Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DuotonePicker, DuotoneSwatch: Convert to TypeScript #49060
DuotonePicker, DuotoneSwatch: Convert to TypeScript #49060
Changes from 12 commits
a1a31bc
66f19f4
4ce6ed1
6bbbb04
e20e675
5b39e04
d4e8260
11295e1
77b648c
6ee7027
ea8ce4f
f786ff3
2c31a5c
e3f041c
8142875
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's definitely a type mismatch at some point here.
ColorListPicker
declares that both itsvalue
and itsonChange
deal withArray< string | undefined >
, whileDuotonePicker
doesn't acceptundefined
in its types.My gut instinct would be to:
DuotonePicker
andColorListPicker
when the value wasundefined
ColorListPicker
againstundefined
values?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, @chad1008 and I took a deeper look at this, and came to the conclusion that in order to fix the TS mismatch properly we'd need to rewrite the component in such a way that enforces
value
andonChange
to deal with a 2 item array (which kind of relates to this other convo ).A quicker way to address the TS error for this PR, while introducing the least amount of runtime changes possible, could be:
ColorListPicker
typesLike this
newColors
array asstring[]
. This is not a 100% correct cast, since in theorynewColors
could still haveundefined
items at index 2 and up, but we kind of know that in practice that's not supposed to happen.Like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into it, Marco + Chad!
I'm not quite convinced that the
string[]
cast is any better than ats-expect-error
— if anything it can imply a false sense of safety. Do you prefer this cast rather than leaving the error for proper fixing later? If not, I can update the TODO comment so it links to this PR thread 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You make a good point — we can leave the
@ts-expect-error
instead of the typecast to avoid the false sense of safety (plus, we will get a TS error if we ever fix those types!).What do you think about the tweaks suggested in the first point above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this: 6ee7027
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me!
Last thing I'd suggest is to improve the comment associated to the
@ts-expect-error
, explaining that it caused by the fact that typescript doesn't know that bothnewColors
andvalue
are in fact supposed to be a 2 item tuple, instead of astring[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ 2c31a5c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered whether it was worth doing a tuple (
[ string, string ]
) here instead ofstring[]
, but decided against it for two reasons:CustomGradientPicker
acceptsstring[]
, so we'll need to do some added type massaging there.[ '#foo', '#bar' ] as const
to make the type checks pass, which is non-obvious.Does that sound reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tuple would be definitely better in representing the actual type that the component is expecting — without it, TS wouldn't be able to detect a malformed
value
(ie.[]
,[ '#fff' ]
etc).But the points that you make are valid, and therefore I'd be ok with typing it as
string[]
, at least initially. We can always narrow the type (or put more runtime checks in place) later as we see fitThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you pointed out recently, the new const type improvements may come handy for this scenario!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not required for this PR, so feel free to ignore)
As we work on color-related components, it would be great to take a wider look at these components' types, and see if we can better highlight dependencies and/or find inconsisntencies.
For example, the
Color
type here (and the type of thecolors
prop for theColorListPicker
component) could be derived from theColorObject
type fromColorPalette
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An interesting example of how some clever functional code may not play nice with TS 😆 The initial values (
[ { brightness: 1 }, { brightness: 0 } ]
) will never be remaining at the point of the final.map()
, but TS cannot know that.I think this is probably the least annoying/invasive way around it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed!
Although annoying at times, I'm glad that TS is able to pick up these edge cases. The fact that the author of these lines of code was clever in writing the original implementation doesn't necessarily mean that a malformed color palette (e.g with out of scale brightness values) or another developer making amends to the algorithm could introduce a bug later in time. TS checks are great in avoiding such events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though for this particular function I would rely on some unit tests more than TS 🫣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true! I guess in my mind I implicitly considered TS static linting as a the first test that runs against the code.