Polymer / shop

The Shop app
https://shop.polymer-project.org/
986 stars 494 forks source link

Debounce 404 page to ensure properties have been updated #110

Closed TimvdLippe closed 6 years ago

TimvdLippe commented 7 years ago

This was a tricky one. The bug boiled down to app-route updating the route segments one at a time. Since bindings are updated sequentially, at first routeData.category would be updated and after all observers etc... were finished, routeData.item would be updated. In consequence, there is 1 moment of inconsistency in the data, since the category was updated, but the corresponding item was not.

To fix this, the actual item retrieval must be debounced 1 frame to make sure both bindings are updated and only then the item has to be computed.

There was a visual regression when switching tabs in shop-detail where it thought that the item was updated too early. E.g. the binding of visible was updated, but since item was debounced it was not ready yet. Therefore that debounce has to run after 2 frames.

Fixes #100

frankiefu commented 7 years ago

@TimvdLippe Thanks for looking into this and the potential fix. The delay in the debounce required in shop-detail is very unfortunate. Also there is still another place where it shows 404 when it shouldn't: https://github.com/Polymer/shop/issues/111

We are receiving many complaints about Shop demo not working because of the 404 changes. I am going to revert the changes that handles 404 for sub routes for now, specifically the firing of the show-invalid-url-warning event in shop-category-data. Let's look into a better way to handle the 404, for example, maybe we should handle it at the page level.

TimvdLippe commented 7 years ago

@frankiefu Understood! Looking forward to your fix to the problem 😄

TimvdLippe commented 6 years ago

I think this PR is no longer necessary.