Support signal values for html + svg attributes #3747
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
marvinhagemeister
requested review from
developit,
JoviDeCroock and
rschristian
Comment on lines
+428
to
+435
| export type CompositionEventHandler<Target extends EventTarget> = | ||
| EventHandler<TargetedCompositionEvent<Target>>; |
There was a problem hiding this comment.
Prettier gonna prettier!
marvinhagemeister
force-pushed
the
signals-type
branch
2 times, most recently
from
41fb88f to
42322ce
Compare
rschristian
approved these changes
Sep 27, 2022
The behavior to resolve these types seems different from the standard behavior from TypeScript. The more elegant way of using generics or clever intersection types doesn't work. The only thing TS seems to pick up is enhancing every property by hand.
marvinhagemeister
force-pushed
the
signals-type
branch
from
42322ce to
eeb5d6c
Compare
|
FWIW: I found a way with generics that requires less typing on our part, but it's worse compared to the changes in the PR because TS doesn't unwrap the type in IDE hints. With the changes in this PR for the With some generic voodoo, it would display: |
JoviDeCroock
approved these changes
Sep 27, 2022
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.
This PR updates our JSX type definitions to support passing signals as attributes. In a perfect world we'd be able to do that from
@preact/signals, but due to the various declaration merging rules and other factors, this would lead to ambiguity and ordering issues. See: microsoft/TypeScript#50808Whilst we could nudge users to copy & paste a type definition file into their own project, this would be less than ideal for newcomers or users who solely use JavaScript. So after a bit of back and forth and a bit chatter with @developit, we both felt that enhancing the types here is the most DX friendly way for our users.
Looking at the changes you might be curios why I went the verbose way of adding the signals type for every single property value instead of relying on generics or clever intersection types. To be honest: This was the only way TypeScript would pick the Signals type up. Whenever I tried any other approach, TS would drop the whole interface resolution completely. This makes me suspicious of them having special behavior for JSX types or something. Not sure. Either way I think this isn't too bad and workable. The DX improvement for end users is definitely there and imo outweighs a slight annoyance on our part.
Fixes preactjs/signals#106