Fix XYZPropsWeControl and cleanup internal TypeScript types
#2329
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
The idea behind the `PropsWeControl` is that we can omit all the fields that we are controlling entirely. In this case, passing a prop like `role`, but if we already set the role ourselves then the prop won't do anything at all. This is why we want to alert the end user that it is an "error". It can also happen that we "control" the value by default, but keep incoming props into account. For example we generate a unique ID for most components, but you can provide your own to override it. In this case we _don't_ want to include the ID in the `XYZPropsWeControl`. Additionally, we introduced some functionality months ago where we call event callbacks (`onClick`, ...) from the incoming props before our own callbacks. This means that by definition all `onXYZ` callbacks can be provided.
Whenever we explicitly provide custom types for certain props, then we make sure to omit those keys first from the original props (of let's say an `input`). This is important so that TypeScript doesn't try to "merge" those types together.
The `console.log` is typed as `(...args: any[]) => void` which means that it will incorrectly mark its incoming data as `any` as well. Converting it to `x => console.log(x)` makes TypeScript happy. Or in this case, angry since it found a bug. This is required because it _can_ be that your value (e.g.: the value of a Combobox) is an object (e.g.: a `User`), but it is also nullable. Therefore we can provide the value `null`. This would mean that eventually this resolves to `keyof null` which is `never`, but we just want a string in this case. ```diff -export type ByComparator<T> = (keyof T & string) | ((a: T, b: T) => boolean) +export type ByComparator<T> = + | (T extends null ? string : keyof T & string) + | ((a: T, b: T) => boolean) ```
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
RobinMalfait
changed the title
XYZPropsWeControl and a bunch of internal TypeScript types
RobinMalfait
changed the title
XYZPropsWeControl and a bunch of internal TypeScript typesXYZPropsWeControl and cleanup internal TypeScript types
RobinMalfait
force-pushed
the
fix/improve-types
branch
from
61aa0ad
to
f5b0add
Compare
+ add `attrs.class` as well
RobinMalfait
force-pushed
the
fix/improve-types
branch
from
b738782
to
478c18e
Compare
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.
Recently we improved the correctness of our TypeScript types. However, some of them started catching the actual bugs we initially intended. For example a
Listbox.Buttonhas an internaltype PropsWeControl = 'disabled'. This is a union of types that we fully control in Headless UI and you can't override them at all. We always intended to let TypeScript "fail" when you pass any of those but this wasn't always the case. Now it is.In fact, that's also what we started seeing in recent issues (#2315, #2327). If you look at the actual code in some of the linked issues then you can see that a
disabledprop is passed to aListbox.Button, but at runtime it didn't work as expected, that's where TypeScript now correctly helps us!But this PR fixes a few things that now also show up. For example, we recently made sure that all event listeners (like
onClick) of the incoming props are executed before the internalonClickevent listeners. This allows us to look at theevent.defaultPreventedproperty to know if we should continue or not. This way you as a developer using Headless UI have more control over those event listeners and how they behave internally.You might have guessed it, but some of those
onClickand other event handlers were also in the list ofXYZPropsWeControl. This is the part that was incorrect and will be fixed in this PR.While going through the types, @thecrypticace and I also cleaned up some internal types so that the builds are a bit cleaner again (they didn't affect public APIs, but always nice to clean things up).
Fixes: #2327