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.

Sign up for GitHub

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

Jump to bottom

Falsey fastDomIf can lead to invalid batched paths #5629

Closed
kevinpschaaf opened this issue Feb 13, 2020 · 0 comments · May be fixed by 137717unity/wpt.fyi#4, eroself/vaadin-litelement-datebook#2, eroself/vaadin-litelement-datebook#3, eroself/vaadin-litelement-datebook#4 or eroself/vaadin-litelement-datebook#5
Closed

Falsey fastDomIf can lead to invalid batched paths #5629

kevinpschaaf opened this issue Feb 13, 2020 · 0 comments · May be fixed by 137717unity/wpt.fyi#4, eroself/vaadin-litelement-datebook#2, eroself/vaadin-litelement-datebook#3, eroself/vaadin-litelement-datebook#4 or eroself/vaadin-litelement-datebook#5
Assignees
@kevinpschaaf
Labels
bug

Comments

@kevinpschaaf
Copy link
Member

Description

When fastDomIf is enabled (legacy-undefined-noBatch branch), path notifications will batch in syncInfo until the dom-if becomes truthy again, at which point all batched path notifications will be included in changedProps. Polymer 2/3 has a rule that paths must not be batched, because the property effect de-duping mechanism will only present the first matching path to an observer, which is unexpected and can cause program errors.

Although there is a fundamental flaw in Polymer 2/3's "falsey dom-if squelchinng" related to path notifications (#4818), fastDomIf mistakenly batches them rather than dropping them like the non-fastDomIf case, and keeps them batched across re-stamped instances, which even when using the workarounds for #4818 (restamp or mutable-data) can still cause errors. The goal for this issue is to match the non-fastDomIf behavior & limitations.

Versions

  • Polymer: unreleased (legacy-undefined-noBatch branch)
kevinpschaaf added a commit that referenced this issue Feb 13, 2020
@kevinpschaaf
Store syncInfo on instance and don't sync paths. Fixes #5629
353eabd 
* Previously the `syncInfo` used to store changed properties while the dom-if was false was stored on the dom-if. This means that even under `restamp`, any stored properties from a previous instance would be applied to the next instance. This change moves the `syncInfo` storage to the instance, so it naturally goes away when the instance is discarded.  Any new instance will take current values from the host.
* Due to limitations described in #4818, it is bad/illegal to allow paths to be batched and played through `runEffects`, since effect de-duping occurs by _root property_, so effects will only be running once for the first path matching an effect, with all other paths being dropped on the ground.  This can be particularly bad if the user e.g. `set` a path to an object, and then subsequently nulled the object; the observer would then be acting on a path that is no longer valid.  This change only stores the root property & value for any paths that come in, which matches the non-`fastDomIf` behavior with only storing `root(prop)` in `__invalidProps`.
@kevinpschaaf kevinpschaaf mentioned this issue Feb 13, 2020
Merged
@kevinpschaaf kevinpschaaf self-assigned this Feb 18, 2020
kevinpschaaf added a commit that referenced this issue Feb 21, 2020
@kevinpschaaf
Merge pull request #5630 from Polymer/legacy-undefined-noBatch-5629
537da37 
Store syncInfo on instance and don't sync paths. Fixes #5629
@snyk-bot snyk-bot mentioned this issue May 5, 2020
Open
This was referenced Sep 7, 2024
Open
Open
This was referenced Sep 14, 2024
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kevinpschaaf kevinpschaaf

Labels
bug
Projects
None yet
Milestone
No milestone
1 participant
@kevinpschaaf