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

app-route should set active property AFTER setting data in place #148

Closed
akc42 opened this issue Sep 23, 2016 · 2 comments
Closed

app-route should set active property AFTER setting data in place #148

akc42 opened this issue Sep 23, 2016 · 2 comments

Comments

@akc42
Copy link
Contributor

akc42 commented Sep 23, 2016

app-route has a mechanism built into it to attempt to set all the property updates that have to take place at the same time. - it attempts to do this by separately setting the value and then notifying the change. Actually there is a bug in the current implementation as raised in #109 and fixed by pull request #132. But even this is insufficient.

Although app-route's properties may all be updated simultaneously as soon as they are passed out of app-route and into a parent element acting as an intermediary they get split up again. So, in the __setMulti function inside app-route the values get set, but are not notified outside of app-route. After all the values are set, there is a loop though the properties that are being updating and a notifyChange is sent to each one. Because active is alphabetically first, it happens to be the one that a notifyChange event is sent to first.

app-route leaves the data property untouched when active is set to false. A design decision designed to prevent irrelevant updates to the dom. So it is natural therefore for a parent element to observe changes to its copy of the active property and to use the transition to true to trigger any activity. Almost certainly the first thing it is likely to do is examine its copy of the data property from app-route. But this value hasn't been updated in the parent yet, because the notify change for the data changes gets fired after the active change.

So I think __setMulti should specifically notify the data change before the active change. I am not sure about tail changes, but given #125 suggests passing active into the tail I think that probably should be notified after the other two.

I am inclined to update #132 to handle this case. Comments?

@akc42
Copy link
Contributor Author

akc42 commented Sep 23, 2016

I see Polymer 2.0 should have proper multi property updates. However I notice the 2.0 preview branch of app-route appears to set the active property before the other properties. This worries me for the reasons explained above.

@e111077
Copy link
Member

e111077 commented Oct 19, 2017

Should be fixed in #218. Issue was that it was set atomically before and no longer in 2.0 since our atomic update did not support read-only props. It does now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants