Svelte 5#1078
Conversation
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
afc9b0d to
565e004
Compare
761f748 to
cb2d9c2
Compare
f4b5f6a to
234a03c
Compare
28cda17 to
1004d96
Compare
2bafc0f to
2966a76
Compare
2ce8501 to
bf6a175
Compare
AlanBreck
left a comment
There was a problem hiding this comment.
Looking really good! I have several "why" questions below, none of which are meant to imply "we shouldn't be doing this".
| type ButtonHTMLAttributes = Pick< | ||
| SvelteHTMLElements['button'], | ||
| 'id' | 'class' | 'style' | 'disabled' | 'type' | 'aria-label' | ||
| >; | ||
|
|
||
| type LinkHTMLAttributes = Pick< | ||
| SvelteHTMLElements['a'], | ||
| 'id' | 'class' | 'style' | 'target' | 'rel' | 'aria-label' | ||
| >; |
There was a problem hiding this comment.
Mainly for the react bindings IIRC, the type was too complex and the compiler couldn't figure it out, details are here: benjaminknox#4
There was a problem hiding this comment.
🤔 This may also be an issue with some other components, so I'm spending a bit of time importing each to the example react app.
There was a problem hiding this comment.
Hmm... ok. It would be ideal not to have to do this since we theoretically would like to expose all of the available button attributes via props spreading, and wouldn't want TS to complain about legitimately used attributes.
There was a problem hiding this comment.
Sounds good, just for reference the types are here: svelte/elements
In svelte 5 it looks like there's more comprehensive types than 4 and combining them here is too much for the compiler, but we may be able to find another way to get the type we want?
I'll be working on this with this draft pr: benjaminknox#6
There was a problem hiding this comment.
Yeah, I think that's a good solution!
There was a problem hiding this comment.
Well... looks like we may be coming back full circle to your initial solution of providing a defined list of allowed attributes. 😅 Despite working in the examples, it still triggers the same error in brave-core.
I did a quick audit of some of my projects, and here are some of the attributes that I'd ideally like to support for Button:
Global
idstylearia-*data-*
Anchor:
hreftargetrel
Button:
typeforform
There was a problem hiding this comment.
Yeah, I played around a bit more and just keep bumping into more and more problems 😢
There was a problem hiding this comment.
Some more properties that'd be helpful:
nametitleautofocustype(for button)
There was a problem hiding this comment.
I'm also hitting issues with navigation item in brave-core, so maybe it need the same treatment
There was a problem hiding this comment.
Why is this file necessary?
There was a problem hiding this comment.
Added for code reuse and because of a svelte 5 type change in the HTMLAttributes type exported by svelte/elements.
This type gets used in:
src/components/buttonMenu/buttonMenu.svelte
src/components/dropdown/dropdown.svelte
src/components/menu/menu.svelte
Svelte 5 added a children prop to HTMLAttributes<T> typed as a Snippet, but these components need children as raw DOM content, so using it caused a type conflict after I upgraded.
* [Collapse]: Add :global() style for details open arrow * [Dialog]: Add :global() style for actions slot * [Alert]: Add :global() style for active selector * [FormItem]: Add :global() style to for :focus-visible wildcard * fix review feedback * upgrade to latest minor version of svelte5 * pin svelte version * [FormItem]: rescope :global around css rules
3d59d1f to
0d781d2
Compare
What?
Svelte5 upgrade branch, branched from #890
Issues:
SnippetTypes of property 'children' are incompatible. Type 'string' is not assignable to type 'Snippet<[]>'