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

Query Parameter Changes that occur simultaneously with path changes not propagated at the same time #149

Open
6 tasks
akc42 opened this issue Sep 27, 2016 · 5 comments

Comments

@akc42
Copy link
Contributor

akc42 commented Sep 27, 2016

Description

When a url changes such that there is a path change and a change of query-parameters, the path, without the query parameters gets propagated around the app-route network of elements and then in a second phase the updated query-params get propagated.

The net result is a observer function looking at changes to the route, doesn't see the correct query parameters when running with the change.

The cause is __computeRoutePath in app-location, since it starts a propagation sequence that doesn't include the simultaneous change to the query params..

Expected outcome

As the route changes it should already have the query parameters in the route.

Actual outcome

The change is propagated with the previous value of query parameters, so any functions expecting to see the query parameters don't see them

Live Demo

github repository akc42/app-route has a branch query-param-order with a test case that shows this problem. Unfortunately my proposed solution (to debounce _locationChanged in app-route-converter breaks lots of other tests so isn't the right one

Steps to reproduce

see test case test-query-order.html and the associated test element app-example-2.html

Browsers Affected

  • [ x] Chrome
  • [ x] Firefox

Not tested the rest

  • Safari 9
  • Safari 8
  • Safari 7
  • Edge
  • IE 11
  • IE 10
@akc42
Copy link
Contributor Author

akc42 commented Sep 28, 2016

I was wrong about the root cause of this issue. The real culprit is iron location which has a method:-

      _urlChanged: function() {
        // We want to extract all info out of the updated URL before we
        // try to write anything back into it.
        //
        // i.e. without _dontUpdateUrl we'd overwrite the new path with the old
        // one when we set this.hash. Likewise for query.
        this._dontUpdateUrl = true;
        this._hashChanged();
        this.path = window.decodeURIComponent(window.location.pathname);
        this.query = window.decodeURIComponent(
            window.location.search.substring(1));
        this._dontUpdateUrl = false;
        this._updateUrl();
      },

The very fact that this.path is set before this.query is the problem. as the first setting of this.path is what propagates the changes without the query value.

A solution might be to change the order of these two settings - although this would then propagate the queryParams change, which might be less problematic, but still wrong.

I wonder if the problem is a design one - that of having the layering of iron-location (and iron-query-params) through app-route-converter to app-location each with separate variables to propagate, effectively simultaneously, and that the solution should really be that app-location create the route object from the window.location components in one step.

@akc42
Copy link
Contributor Author

akc42 commented Oct 5, 2016

Sadly I have come to the conclusion that its a design issue that isn't going to go away. I have ended up making my own pair of elements <akc-location> and <akc-route> in my own github account that approximate to <app-location> and <app-route> But which solve these issues by carrying all the data around in the route object. See https://github.com/akc42/akc-route

@robrez
Copy link

robrez commented Dec 1, 2016

I'm having a problem getting correct query-params values behavior from nested app-routes...

This works fine:

<dom-module id="x-app">
  <template>
    <app-route route="{{route}}" pattern="/:section" data="{{routeData}}"
            tail="{{subroute}}"></app-route>
    <app-route route="{{subroute}}" pattern="/:page" data="{{subrouteData}}"
            query-params="{{queryParams}}"></app-route>
  </template>
  <script>
      /* ... */
      observers: [ '_onRoutingChange(subrouteData, queryParams)' ]
  </script>
</dom-module>

In my case, the 2nd route element is in a composed child element...

    <!-- in x-app -->
    <app-route route="{{route}}" pattern="/:section" data="{{routeData}}"
            tail="{{subroute}}"></app-route>
    <x-foobar subroute="{{subroute}}"></x-foobar>

... the 2nd app-route element moved into x-foobar --
<dom-module id="x-foobar">
  <template>
    <app-route route="{{subroute}}" pattern="/:page" data="{{subrouteData}}"
            query-params="{{queryParams}}"></app-route>
  <template>
  <script>
      /* ... */
      observers: [ '_onRoutingChange(subrouteData, queryParams)' ]
  </script>
</dom-module>

In the broken case, queryParams ends up being a stale value...

If I modify the overall structure such that the x-app element passes queryParams into the x-foobar child as a property, then everything also works. Not sure how much this may or may not relate to what akc42 reported

@rene-lindner-isw
Copy link

rene-lindner-isw commented Jul 3, 2017

Run into the same issue.
If the url changes from "a" to "b?c=0" you would expect the following events: path-changed, query-params-changed, route-changed.
But you get path-changed, route-changed, query-params-changed, route-changed.

The url is only changed 1 time, but route-changed is fired 2 times. (Haven't took a look at it, but most likely because these are the default notify events, and path and query-params are set separate).

@czellweg
Copy link

czellweg commented Jul 18, 2017

This PR might be interesting: PolymerElements/iron-location#86

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

4 participants