You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#91 and #102 caused several regressions (and uncovered some existing bugs). Some of which we fixed with subsequent PRs, but generally piling fixes on top of existing broken code tends to produce worse results and higher complexity than going back, fixing the spec, and reimplementing.
So I think we need to bite the bullet and do just that.
What makes the rollback tricky is that once we discovered the regressions, we added tests for them. These need to stay. FakeElement too (or use a library that does it properly). But simply keeping all the tests from the rewrite is a no-go, since many depend on internal implementation details of the rewrite (a problem in itself).
Also, there are pre-existing bugs we discovered and fixed since then.
I propose this plan:
We roll back to e344b1f (last commit before signals)
Let's create a list of requirements here for behavior we want to preserve and ask Claude to create tests for them
Fix any pre-existing bugs, and add tests to prevent regressions
Setting a value explicitly that happens to be the same as the default DOES call setAttribute
removeAttribute collapses to the default.
Reflection:
Property write reflects to the attribute.
Pre-set attribute coerces into the typed property on mount.
reflect: { from, to } honors asymmetric attribute names.
Computed props (get):
get returns the derived value on read.
Writing a tracked dep re-runs get and fires propchange for the derived prop.
Other:
A value set as a data property on an element before its prototype accessor exists is preserved, goes through parse correctly, and any props depending on it have correct values
Setting different values that are the same after conversion do not fire propchange
#91 and #102 caused several regressions (and uncovered some existing bugs). Some of which we fixed with subsequent PRs, but generally piling fixes on top of existing broken code tends to produce worse results and higher complexity than going back, fixing the spec, and reimplementing.
So I think we need to bite the bullet and do just that.
What makes the rollback tricky is that once we discovered the regressions, we added tests for them. These need to stay. FakeElement too (or use a library that does it properly). But simply keeping all the tests from the rewrite is a no-go, since many depend on internal implementation details of the rewrite (a problem in itself).
Also, there are pre-existing bugs we discovered and fixed since then.
I propose this plan:
Bugs to fix before implementing signals
setAttribute()after mount does not update the property on plainreflectprops #98propchangeevents queued while disconnected are not dispatched on reconnect #100We should also add tests for these behaviors.
Behavioral baseline (tests to add before reimplementing)
First, we should find a library instead of implementing FakeElement ourselves and add it as a dev dependency. This cannot be a new problem.
These should all pass on e344b1f. They're the regression net protecting consumer-observable behavior through the signals refactor.
Basic API (mostly already present in
test/Prop.jsconstructor() group — keep what's there, expand only where gaps exist):type,default,reflect, etc.reflect: true | false | "name" | { from, to }parses correctly intofromAttribute/toAttribute.Props.observedAttributesderives from reflecting props.Props.add(name, spec)andProps.add({...})both work.Sync IDL semantics to prevent #111
el.x = v; el.xreturnsvsynchronously.el.x = 1; el.x = 2; el.x = undefinedfires threepropchangeevents in order, synchronouslyel.x = v, reading any prop derived fromx(viaget,convert, or functiondefault) returns the new derived value on the same turn.el.setAttribute("x", "foo"),el.x === "foo"synchronously.Defaults:
default()is reactive: re-runs when its dependencies changedefaultPropmirrors another prop's value; severs on explicit write; restores onundefined.setAttributeon mount. (prevent Race: external read of a Computed-backed prop beforeinitializeForpollutes the attribute and clobbers programmatic writes #105)setAttributeremoveAttributecollapses to the default.Reflection:
reflect: { from, to }honors asymmetric attribute names.Computed props (
get):getreturns the derived value on read.getand firespropchangefor the derived prop.Other:
parsecorrectly, and any props depending on it have correct valuespropchange