-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multi-spread #1289
Multi-spread #1289
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1289 +/- ##
==========================================
- Coverage 91.65% 91.64% -0.02%
==========================================
Files 122 124 +2
Lines 4328 4752 +424
Branches 1364 1557 +193
==========================================
+ Hits 3967 4355 +388
- Misses 148 166 +18
- Partials 213 231 +18
Continue to review full report at Codecov.
|
Taking the WIP off this, if anyone wants to take a look. Rather than anticipating all the possible failure modes, my inclination is to get this into people's hands soon so that we can mop up any bugs. The above-mentioned optimisations can go into a follow-up PR |
(Worth a mention: the codebase badly needs to be refactored, particularly all the stuff around |
Very incomplete, but I think this is a solid direction to go in. It solves #195 in a way that aligns with people's expectations from React (multiple spread properties can exist on an element or component, interspersed with named attributes/properties) but preserves Svelte's mechanism for avoiding unnecessary work even when dealing with grubby mutable data, and it doesn't break the bank in terms of generated code.
TODO:
lotssome more tests{ "data-foo": "static" }
in the update block when we can just reuse the previous object, likediv_spread_levels[1]
. We could also possibly optimise the case where there is just one spread prop, or one spread prop followed by a bunch of static props. Doesn't have to be in this PR thoughSomething else we might want to consider is whether to set properties instead of attributes where applicable (e.g.
button.disabled = false
will work;button.setAttribute("disabled", "false")
will not). I think it can be done just by testing for the existence of the property on the element......though would have to test that. (I think we're ok viz
class -> className
etc; I believe all the attributes that fall into that category are string attributes, so they should be ok. Again, need to test/check.) Also, things likeoption.__value
might warrant special treatment.Will finish this PR off soon when I get time.