Skip to content
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

refactor(forms): Update computed properties to live in AbstractContro… #56545

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Jun 21, 2024

…lStatus

The computed properties are not "safe" to share around broadly because form status, value, etc. is not stable during change detection. If these were shared more broadly, recomputation happens when the value is requested at these different read locations. This can result in the value flipping back and forth before settling on its final state, which may actually be the same as it was at the start of change detection. The host bindings rely on being able to check the computed equality against what it was on its own last read.

@atscott atscott added area: forms target: minor This PR is targeted for the next minor release labels Jun 21, 2024
@ngbot ngbot bot added this to the Backlog milestone Jun 21, 2024
@pullapprove pullapprove bot requested a review from AndrewKushnir June 21, 2024 16:46
…lStatus

The `computed` properties are not "safe" to share around broadly because
form status, value, etc. is not stable during change detection. If these
were shared more broadly, recomputation happens when the value is
requested at these different read locations. This can result in the
value flipping back and forth before settling on its final state, which
may actually be the same as it was at the start of change detection. The
host bindings rely on being able to check the computed equality against
what it was on its own last read.
@AndrewKushnir AndrewKushnir requested review from dylhunn and removed request for AndrewKushnir June 23, 2024 19:36
@kbrilla
Copy link

kbrilla commented Jun 25, 2024

@eneajaho this could make into interesting blog about signal gotchas - My question is why is there a difrerencle where the computed property is created and how many times is read? Does it have something to do with untracked(()=> this.someSignal.set('xyz')?

@atscott
Copy link
Contributor Author

atscott commented Jun 25, 2024

@kbrilla It doesn't exactly have anything to do with untracked, though the use of untracked in all the sets for the signal indicates that the forms package is generally doing things in a way that's not really recommended. The forms package has always had these weird types of problems with unintuitive workarounds (i.e. onlySelf and emitEvent booleans to "hide" updates from observers, which you can't do with signals).

It's not really about "how many times" something is read, but the fact that computed's are lazy and only recompute when the value is requested. The forms properties are both requested and rewritten all over the place during change detection. What's allowing the host bindings to work correctly with computed today in a way that doesn't constantly re-mark the host for check is that it's the only place that reads the computed property. So the fact that its value would have changed back and forth many times before the next read doesn't matter. This is essentially #56071 and we're still discussing how we want to approach this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: forms target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants