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

Fix of props update order, inactive while updating if data changed #222

Closed
wants to merge 3 commits into from

Conversation

iSuslov
Copy link

@iSuslov iSuslov commented Feb 16, 2018

@iSuslov iSuslov requested a review from e111077 as a code owner February 16, 2018 01:00
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@googlebot
Copy link

CLAs look good, thanks!

@iSuslov iSuslov changed the title Update order fix ("data" first, then "active", only after that "tail") Fix of props update order, inactive while updating if data changed Feb 16, 2018
Copy link
Contributor

@e111077 e111077 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the submission. Unfortunately this is breaking some of our integration and unit tests.

I think the right approach would be to unfortunately do another hack like __setMulti. Perhaps running the internals of setProperites in the correct order may notify in the desired order.

i.e. run _setPendingPropertyOrPath for each of the new values in the correct order and call _invalidateProperties. If that doesn't work we may have to dig in more and find the correct internal notification functions.

@iSuslov
Copy link
Author

iSuslov commented Feb 16, 2018

Hi, your idea is reasonable and I will try it. I made another commit, which probably will fail unit testing again, the idea is to set app-route to inactive state while updating properties if data changed and then bring it back to active. Why it is important: see updated #164 (comment)

@iSuslov
Copy link
Author

iSuslov commented Feb 16, 2018

Well, using internals of 'setProperties' does not help really, since it is still unordered on lower level https://github.com/Polymer/polymer/blob/5c02730974f8e3138405e596cb5f8d8bb388a41f/lib/mixins/property-effects.html#L124

// atomic update
this.setProperties(propertyUpdates, true);
//Update properties in a specific order: data, tail and set inactive during update if data changed
propertyUpdates.hasOwnProperty("data") && this._setPendingPropertyOrPath("data", propertyUpdates.data, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about setting without notifying all of the properties:

this._setPendingPropertyOrPath("data", propertyUpdates.data, false);

and then call this.notifyPath('data', propertyUpdates.data) but in the correct order after all have been set?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure this will work; not sure if it will trigger runEffects

@e111077
Copy link
Contributor

e111077 commented Mar 8, 2018

Any updates?

@mercmobily
Copy link

Ho there. I am following this issue from #164 which is still basically unresolved... and it looks like it will be for a while. Any hints on this?

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

Successfully merging this pull request may close these issues.

4 participants