DataViews: Type all the filters components #61795
Merged
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
youknowriad
added
[Type] Code Quality
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
youknowriad
commented
May 20, 2024
| if ( operators.includes( 'notIn' ) ) { | ||
| operators = operators.filter( ( operator ) => operator !== 'notIn' ); | ||
| operators.push( 'isNot' ); | ||
| } |
There was a problem hiding this comment.
I had confirmed previously with @oandregal that these are not used anymore.
There was a problem hiding this comment.
I've prepared a PR that adds a changelog note about this, to let consumers of the package know about the breaking changes: #62013
|
Size Change: -55 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
sirreal
reviewed
May 21, 2024
| @@ -3,49 +3,6 @@ | |||
| */ | |||
| import { __ } from '@wordpress/i18n'; | |||
|
|
|||
| // Filter operators. | |||
There was a problem hiding this comment.
I wrote a big comment about how we can rely more on the type system and make a lot of these constants irrelevant without sacrificing clarity or readability (in my opinion).
Instead of writing the comment, I pushed a change that implements that idea. Please feel free to keep it or revert it, but it's a powerful concept and a compelling way to rely on the information the type system provides.
sirreal
approved these changes
May 21, 2024
There was a problem hiding this comment.
I pushed a change of my own that you can keep or revert. I've left several notes and suggestions as well.
One thing I noticed in my change is there were key properties for operators ({ key: 'is-not-all-filter' }), but those don't seem to be used anywhere. When I they object that included them nothing complained 🤷♂️
packages/dataviews/src/utils.ts
Outdated
Comment on lines
34
to
37
| // Make sure only valid operators are used. | ||
| operators = operators.filter( ( operator ) => | ||
| ALL_OPERATORS.includes( operator ) | ||
| ); |
There was a problem hiding this comment.
I removed this, I'm not sure whether operators can enter this system and potentially be invalid.
If operators are internal, then the check is likely redundant. If not, then a check like this would be good to restore. A type guard like this would work:
export function isValidOperator( candidate: string ): candidate is Operator {
switch ( candidate ) {
case 'is':
case 'isNot':
case 'isAny':
case 'isNone':
case 'isAll':
case 'isNotAll':
return true;
}
return false;
}There was a problem hiding this comment.
They can enter the component and be invalid but it's doing it wrong.
| @@ -39,8 +46,11 @@ function normalizeSearchInput( input = '' ) { | |||
| return removeAccents( input.trim().toLowerCase() ); | |||
| } | |||
|
|
|||
| const EMPTY_ARRAY = []; | |||
| const getCurrentValue = ( filterDefinition, currentFilter ) => { | |||
| const EMPTY_ARRAY = [] as []; | |||
There was a problem hiding this comment.
This is a very minor point here, but I'd recommend avoiding type assertions (x as T) as much as possible. In this case, an annotation should be sufficient:
Suggested change
| const EMPTY_ARRAY = [] as []; | |
| const EMPTY_ARRAY: [] = []; |
They're very similar, especially in this case, but the assertion tells the compiler we know better, whereas the annotation is more of a hint. Assertions can be dangerous:
const x: number[] = [1];
const emptyAssert = x as []; // no type error
const emptyAnnotate: [] = x; // type error
// Type 'number[]' is not assignable to type '[]'.
// Target allows only 0 element(s) but source may have more.
packages/dataviews/src/filters.tsx
Outdated
| view.filters.some( | ||
| ( f ) => | ||
| f.field === field.id && | ||
| ALL_OPERATORS.includes( f.operator ) |
There was a problem hiding this comment.
This is the other place I removed the operator validation. Again, if the operator is internal this should be fine. If they come from outside, it would be good to check them.
There was a problem hiding this comment.
They come from outside
| } ) { | ||
| const toggleRef = useRef(); | ||
| }: FilterSummaryProps ) { | ||
| const toggleRef = useRef< HTMLDivElement | null >( null ); |
There was a problem hiding this comment.
React uses a little trick here to differentiate between refs that we update manually and refs that the system manages. This seems like a ref the system manages. If we make this change:
Suggested change
| const toggleRef = useRef< HTMLDivElement | null >( null ); | |
| const toggleRef = useRef< HTMLDivElement >( null ); |
React recognizes that it's a RefObject instead of a MutableRefObject. This basically allows assignment to current or not:
// Ok
const toggleRef = useRef< HTMLDivElement | null >( null );
toggleRef.current = document.createElement( 'div' );
// Not OK (not mutable)
const toggleRef = useRef< HTMLDivElement >( null );
toggleRef.current = document.createElement( 'div' ); // type error
|
Flaky tests detected in b838662. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9178593900
|
youknowriad
commented
May 21, 2024
| @@ -55,6 +46,28 @@ interface FilterSummaryProps extends OperatorSelectorProps { | |||
| openedFilter: string | null; | |||
| } | |||
|
|
|||
| function getOperatorLabel( operator: Operator ): string { | |||
There was a problem hiding this comment.
Personally I prefer records over functions like this but it's a cosmetic difference, that's fine by me.
There was a problem hiding this comment.
I agree, it's mostly a superficial change. My experience has been that simple functions like this and really switches and unions work very well together and complement TypeScript's type system. A properly typed record object is probably just as good, but I've seen folks run into type issues more often with record objects.
|
@sirreal I hope you don't mind, but I'm going to revert your commit for now. Not that I don't want it at all, it's just that it contains some changes that slightly change behavior (around handling invalid operators) that I'm not entirely certain about. I think we can definitely apply it in its own PR though and test properly. |
youknowriad
force-pushed
the
add/filter-types
branch
from
55353fe
to
b838662
Compare
|
Yep, that's fine. |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jun 4, 2024
Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
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.
Related #55083
Follow-up to #61185
What?
The DataViews package is a types heavy package. There's a lot of structures that need to be documented properly. So this continues the effort to add explicit types to the package. This PR focuses on typing the several filters components we have. This is probably the latest PR before typing the root "DataViews" component.
Testing instructions
There should be no real change here, it's mostly code quality and documentation.