NumberControl: tidy up and fix types around value prop
#45982
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
Size Change: +261 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
refactor/number-control-value-type
branch
from
d2366f8
to
d211a80
Compare
This was referenced Nov 23, 2022
NumberControl: refactor styles/tests/stories to TypeScript, replace fireEvent with user-event
#45990
Merged
ciampo
changed the base branch from
trunk
to
refactor/number-control-tests-stories-typescript
ciampo
force-pushed
the
refactor/number-control-tests-stories-typescript
branch
from
f644ed5
to
f75fcd0
Compare
DAreRodz
added
[Package] Components
ciampo
force-pushed
the
refactor/number-control-tests-stories-typescript
branch
2 times, most recently
from
98d8b1d
to
6732efb
Compare
Base automatically changed from
refactor/number-control-tests-stories-typescript
to
trunk
November 25, 2022 23:31
ciampo
force-pushed
the
refactor/number-control-value-type
branch
from
d211a80
to
b815bd3
Compare
10 tasks
There was a problem hiding this comment.
I know there is still work in progress for this PR but I had a moment to give it a preliminary test.
It's looking good so far.
- Other than the Uncontrolled storybook example, there weren't any typing errors
- Unit tests pass (NumberControl, UnitControl, RangeControl, ColorPicker etc)
- Controls all appear to be working ok in the Storybook and editor
is changing the type of onCHange args from number to string a breaking change, or a bug fix?
Tricky question to answer. Can it be both?
I'm biased given previous issues faced around this typing problem so I tend to lean towards it being a bug fix. It could easily be seen as a breaking change though for consumers that depended on the previous type.
ciampo
force-pushed
the
refactor/number-control-value-type
branch
from
b815bd3
to
0fa0e87
Compare
ciampo
requested review from
stokesman,
mirka,
noisysocks,
andrewserong and
tyxla
|
PR is now ready for review — I've updated the description with more details :) |
ciampo
commented
Dec 2, 2022
Comment on lines
103
to
145
| describe( 'ensureString', () => { | ||
| it.each( [ | ||
| [ '1', '1' ], | ||
| [ 'abc', 'abc' ], | ||
| [ 2e3, '2000' ], | ||
| [ 42, '42' ], | ||
| [ -14, '-14' ], | ||
| [ 0, '0' ], | ||
| [ '0', '0' ], | ||
| [ '', '' ], | ||
| [ NaN, 'NaN' ], | ||
| ] )( | ||
| 'should convert `%s` (unknown) to `%s` (string)', | ||
| ( input, expectedOutput ) => { | ||
| expect( ensureString( input ) ).toBe( expectedOutput ); | ||
| } | ||
| ); | ||
| } ); | ||
|
|
||
| describe( 'ensureNumber', () => { | ||
| it.each( [ | ||
| [ '1', 1 ], | ||
| [ 'abc', NaN ], | ||
| [ 2e3, 2000 ], | ||
| [ 42, 42 ], | ||
| [ -14, -14 ], | ||
| [ 0, 0 ], | ||
| [ '0', 0 ], | ||
| [ '', NaN ], | ||
| [ 'NaN', NaN ], | ||
| ] )( | ||
| 'should convert `%s` (unknown) to `%s` (string)', | ||
| ( input, expectedOutput ) => { | ||
| expect( ensureNumber( input ) ).toBe( expectedOutput ); | ||
| } | ||
| ); | ||
| } ); |
There was a problem hiding this comment.
The value prop in NumberControl will now be converted from string to a number, and back to a string.
Currently ensureString( ensureNumber( valueAsAString) ) ) !== valueAsAString for some edge cases: in particular, currently ensureNumber('') === NaN, but ensureString( NaN ) === 'NaN').
I was wondering if ensureString( NaN ) should return '' instead, and if that would have any ripercussions? (currently these utilities are only used by NumberControl, but I'd like for them to be used by other components dealing internally with numbers for better consistency)
There was a problem hiding this comment.
It's simply a reality that a string cannot always be turned into a number. Can we have the type of ensureNumber reflect this?
function ensureNumber( value: string ): number | nullThe type system will then guide users away from writing (incorrect) code that doesn't handle invalid strings.
There was a problem hiding this comment.
Given null isn't a number would we also need to tweak the name of ensureNumber in that case?
There was a problem hiding this comment.
Given null isn't a number would we also need to tweak the name of ensureNumber in that case?
Yeah, I could rename that function to ensureValidNumber or ensureFiniteNumber. Any thoughts?
The type system will then guide users away from writing (incorrect) code that doesn't handle invalid strings.
I went ahead and changed ensureNumber and ensureString to return null when they are dealing with non-finite numbers (1da7361).
Not sure about the naming, but otherwise how does that feel like?
There was a problem hiding this comment.
I just realized that these changes break some unit tests, I'll have to look into that too.
Edit: force-pushed a new version of the same changes which should pass unit tests
…reString` functions
…mpty check" before calls to these functions, wrap results in "ensureString"
ciampo
force-pushed
the
refactor/number-control-value-type
branch
from
5a3f164
to
1c0afa0
Compare
|
Hey all! Back from some AFK, I've rebased and started to address some of the feedback. I will try to get to all feedback in the upcoming days |
@noisysocks Trying to interpret what 1.5 months ago Marco meant: since the convention is to
we need to be careful in the way we convert from string to number and viceversa especially around those falsey values ( Hope that present Marco is making sense of past Marco 🔮
I'll to take a deeper look at this, thank you for flagging @aaronrobertshaw ! |
ciampo
force-pushed
the
refactor/number-control-value-type
branch
from
1c0afa0
to
38c1a2e
Compare
…on-finite numbers
ciampo
force-pushed
the
refactor/number-control-value-type
branch
from
38c1a2e
to
3ac25c9
Compare
Right, yeah. Maybe a good exercise would be to complete this table:
|
|
Flaky tests detected in bc58827. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3937911316
|
Inspired by this, I've started working on a separate PR to improve |
ciampo
commented
Jan 18, 2023
| export const ensureNumber = ( value ) => { | ||
| return typeof value === 'string' ? stringToNumber( value ) : value; | ||
| export const ensureFiniteNumber = ( value ) => { | ||
| if ( typeof value === 'number' ) { |
There was a problem hiding this comment.
Note to self: Make sure number is finite here too
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
NumberControl: introducestringas a type for thevalueprop #45949@wordpress/componentspackage to TypeScript #35744Refactor internals of
NumberControlwith the goal of improving its types (ie. remove@ts-expect-errorcomments) and set the ground for the deprecation of thenumbertype for thevalueprop.Why?
See #45949 for more details about the work on
NumberControlHow?
I recommend reviewing this PR commit by commit.
value-related variables from/tostring(this also fixed TypeScript errors)Question: I'm not sure about how to represent / convert falsey values (
'',undefined,null,NaN...) betweenstringandnumber, since some of them have "special" meanings:undefinedtells the component to switch to uncontrolled mode''therefore is used to tell the component that the value is empty, but in controlled modeNaNis a validnumberthat could be returned when parsing strings — should we guard explicitly against it? And how?Testing Instructions
Test
NumberControland its derived components (UnitControl,RangeControl,ColorPicker,AnglePickerControl...) both in Storybook and across the editor (mostly in the post and site editor sidebars).Pay particular attention to:
trunk:isRequiredpropisPressEnterToChangemin) and then pressing enter and / or blurring the input fieldtrunk(I've also added a specific story for the uncontrolled version of the component)The goal of this refactor is to improve the code without introducing runtime changes / regressions. The only known runtime change (bug fix) is that the
onChangecallback in this PR always has astringas its argument (while ontrunkthat would sometimes be anumber)