Skip activating new route if the current route will not change

[pdf]

This avoids reloading and re-rendering unnecessarily. This is particularly useful if you have nested routers where the inner router handles lower url segments and the wrapper handles upper fragments, ie:

<app-router id="outer">
 <app-route path="/" template="/home-page.html"></app-route>
 <app-route path="/user/*" template="/user-page.html"></app-route>
</app-router>

user-page.html:

<app-router id="inner">
 <app-route path="/user/profile" template="/user-profile.html"></app-route>
 <app-route path="/user/stats" template="/user-stats.html"></app-route>
</app-router>

Without this change, navigating between user pages will cause the outer route to activate, followed by the inner route. With this change, navigating between user pages will only trigger the inner routes, and skip re-rendering the containing user-page.html.

[pdf]

I didn't include the minified versions in the PR, for easier merging.

[erikringsmuth]

Yeah, this came up before but I didn't decide what to do at that point https://github.com/erikringsmuth/app-router/issues/54#issuecomment-67163293. I want to have the default behavior reload the entire page. This is the safest thing to do if people are relying on events like ready() and detached() to set up and tear down the current view. The other two cases are re-bind the model, and don't do anything. These could be handled by an attribute like <app-route handle-match="reload|rebind|no-op"></app-route>. Not necessarily that name, but that concept.

The other thing is that custom elements and templates bind the model differently. We'd need to take both cases into account.

[pdf]

I'm happy to stick this behind a param, can you elaborate on your last comment? I'm not convinced the current way templates/elements are bound is correct currently, because two-way binding doesn't appear to work. I was going to open another issue once I get time to work that out.

EDIT: Also, you don't want to rebind the entire model, because changes may have occurred on other model properties that you're not setting from the router, you just want to update those properties that the router is handling explicitly.

[erikringsmuth]

custom element data binding https://github.com/erikringsmuth/app-router/blob/master/src/app-router.js#L285

template data binding https://github.com/erikringsmuth/app-router/blob/master/src/app-router.js#L300

createModel() https://github.com/erikringsmuth/app-router/blob/master/src/app-router.js#L308 gets called right before both of these which is where the before-data-binding event happens.

It's not proper two-way data binding on the model itself, but properties in the model added in before-data-binding should do two way data binding. The URL parameters won't two way data bind. This is the best way I've been able to get it to work given that this whole library works without Polymer as well.

[pdf]

properties in the model added in before-data-binding should do two way data binding

Have you verified that this is the case? I haven't tried with templates yet, but this doesn't appear to work for elements. My suspicion after a cursory inspection a couple of days ago is that createInstance may be required in both cases.

[benjaminapetersen]

Ah, this is interesting. I actually expected data binding on existing DOM would take effect on a match, rather than a destroy/recreate. Example:

paths:
orders/12345
orders/6789
orders/abc

matches:
<app-route
 path="/order/:orderId"
 import="components/route-app-pages/page-order-secondary.html"></app-route>

But this causes a full render, throwing away all that DOM when the DOM is fine & only needs to update minimally. Sadly, this can destroy secondary state that doesn't need to be tracked in URL but is beneficial for user experience.

Seems like app-router just needs a few additional events to make everything possible. I haven't dug into the source yet, but I'm envisioning bindings like this:

// typical events 
created: function() {},
ready: function() {},
domReady: function() {}
remove: function() {},
// These events fire when the DOM is not destroyed, but provide hooks
// for any  updates you need to make if route parameters are the only change.
beforeRender: function() {},
render: function() {},
afterRender: function() {}

Just throwing the ideas out there. Am going to check the app-router source now to see what you have. I know there are additional events being fired already, but I don't think there are events that fit this use case.

[benjaminapetersen]

Here is a quick little snippet of an arbitrary example:

<link rel="import" href="../paper-button/paper-button.html">

<polymer-element name="page-order" attributes="orderId foo bar">
  <template>
    <p><strong>Normal:</strong></p>
    <p>
      Order ID is {{orderId}}.
    </p>
    <p>
      Arbitrary query params, foo: {{foo}}, bar: {{bar}}.
    </p>
    <p>
      Simple example of state within the page that isn't URL encoded,
      but improves user experience.  I would only want these to change
      on a new route.
    </p>
    <p>created: {{created}}.</p>
    <p>ready: {{created}}.</p>
    <p>domReady: {{created}}.</p>
   <p>
     And finally, might as well be able to arbitrarily update something on a 
     route change.  Still may/may not be important enough for URL encoding,
     but may be handy in certain circumstances:  {{routeChanged}}.
   </p>
    <paper-button>Useless button</paper-button>
  </template>
  <script>

    var random = function(min, max) {
      min = min || 1;
      max = max || 1000;
      return Math.floor(Math.random() * max) + min;
    }

    Polymer({
      orderId: undefined,
      // These events create arbitrary data that I'd expect to survive
      // on additional route matches, but be destroyed once the route
      // changed & the DOM needed to be updated.
      created: function() {
        this.created = random();
        console.log('page-order.created()', this.created);
      },
      ready: function() {
        this.ready = random();
        console.log('page-order.ready()', this.ready);
      },
      domReady: function() {
        this.domReady = random();
        console.log('page-order.ready()', this.domReady);
      }
      // route changed can run every time there is an update
      // this is probably better example than my 
      // beforeRender(), render(), afterRender() in the above comment
      routeChanged: function() {
         this.routeChanged = random();
        console.log('page-order.routeChanged()', this.routeChanged);
      }
    });
  </script>
</polymer>

Throwing it out there, hopefully helps think through this item.

[erikringsmuth]

I still want the default behavior of matching the same route to tear down the current page and create a new custom element. I want people to be able to write plain Polymer or vanilla JS as much as possible and not have to explain a new routeChanged() function that replaces ready() only in the case where the page didn't get torn down and replaced.

Suppose we have these options.

<app-route handle-match="reload|rebind|no-op"></app-route>

Then you can use activate-route-end to call the ready() or your user defined routeChanged() function.

<polymer-element name="demo-app">
  <template>
    <app-router id="router">
      <app-route path="/user/:userId" import="/pages/user-page.html" handle-match="rebind" on-activate-route-end="{{userPageLoaded}}"></app-route>
      <app-route path="*" import="/pages/not-found-page.html"></app-route>
    </app-router>
  </template>
  <script>
    Polymer('demo-app', {
      userPageLoaded: function(event) {
        event.detail.activeRoute.ready();
        // or
        event.detail.activeRoute.routeChanged();
      }
    });
  </script>
</polymer-element>

[benjaminapetersen]

I think thats fair, provides the possibility for both && simplest behavior is default behavior. Seems like a win-win!

[benjaminapetersen]

The no-op option makes sense as well, hadn't thought about it, but could definitely be situations where nothing needs to happen at all. A 404 example could be relevant, may not need to do anything at all if the user keeps entering weird paths.

[erikringsmuth]

My only issue is this attribute and options don't seem very self-descriptive to me.

<app-route handle-match="reload|rebind|no-op"></app-route>

An example default case would be something like this where you need to create and clear a JS interval.

router

<app-route path="/server/:serverId" element="server-page"></app-route>

page

<polymer-element name="server-page" attributes="serverId">
  <template>
    ...
  </template>
  <script>
    Polymer({
      domReady: function() {
        this.intervalHandle = setInterval(function() {
          // use the serverId to make an AJAX call that polls the server every 5 seconds
        }, 5000);
      },
      detached: function() {
        clearInterval(this.intervalHandle);
      }
    });
  </script>
</polymer-element>

In this case re-binding could also update the AJAX call's server ID, but I wouldn't expect that to handle all scenarios. The standard custom element lifecycle events are really nice to hook into and rely on.

[benjaminapetersen]

Great point, for avoiding memory leaks. Forgive the chicken straches, throwing ideas out there:

<polymer-element name="server-page" attributes="serverId">
  <template>
    ...
  </template>
  <script>
    Polymer({
      intervalHandle: null,
      domReady: function() {
        // actually dont need this here, really should wait for the attribute change event?
        // this.setupPoll();
      },
      // This event should fire wether this is the first time the object has rendered,
      // or it is a second/third render (rebind) 
      serverIdChanged: function() {
          this.clearPoll();
          this.setupPoll();
      },
      detached: function() {
        this.clearPoll();
      },
      setupPoll: function(oldVal, newVal) {
          if(!newVal) return;
          this.intervalHandle = setInterval(function() {
              // use the serverId to make an AJAX call that polls 
              // the server every 5 seconds
            }, 5000);
      },
      clearPoll: function() {
          if(!this.intervalHandler) return;
          clearInterval(this.intervalHandle);
      }
    });
  </script>
</polymer-element>

I think that this covers the issue of the setInterval getting away while still using native events && attribute binding.

[erikringsmuth]

There are definitely other ways to do the set/clear interval. That's just an example of a use of domReady() and detached(). I don't want to force people to use other functions when the default ones work fine for the 95% use case. Multiple routers is really the special case.

[benjaminapetersen]

Great discussion!

Multiple routers is definitely a special case, but the goal of avoiding unnecessary renders & preserving DOM whenever possible is a big performance advantage (and I think is the real goal here), even if you don't use nested routers (as cool as that is & was a 'wow' factor when I was prototyping with app-router). Since DOM manipulation is generally considered expensive and to be avoided as much as possible, I think this is definitely the right direction!

I think an argument could be made for user experience as well. If a route matches a component to render that is highly complex/nested, but a second hit should initially only change a few variables on that component, preservation is key. Users don't typically like to see excessive spinners/loading/rendering indicators if they can be avoided.

Example, the primary app I am currently working on depend far more on attribute binding than the lifecycle callbacks due to complexity. Since I'm using sockets/webrtc, the page is rarely static. Data-binding to save DOM is essential in this use case. I need a route change to only trigger rendering on the essentials.

Thanks for the quick follow-up!

[benjaminapetersen]

Let me throw out an attempted real world(-ish) example. This is just some code that gives the user a list of options to click to show a certain number of (presumably complex) items to view.

<!-- 
  Menu to provider user choices
-->
<menu>  
  <a href="view/10">View 10 items</a>
  <a href="view/25">View 25 items</a>
  <a href="view/50">View 50 items</a>
  <a href="view/100">View 100 items</a>
</menu>

<!-- 
  Router somewhere in app
-->
<app-router>

  <app-route
    path="view/:count"
    element="items-list-view"></app-route>

  more routes....

</app-router>

<!-- 
  And finally, the list view, importing core-ajax to make requests
-->
<link rel="import" href="../core-ajax/core-ajax.html">
<polymer-element 
  name="items-list-view" attributes="count">
  <template>

    <core-ajax
      id="request"
      url="{{url}}"
      handleas="json"
      on-core-response="{{handleResponse}}"
      on-core-error="{{handleError}}"></core-ajax>

    <template repeat="{{item, i in items}}">
      <!-- 
        Some complex DOM representing an item.  Perhaps invoices or 
        products in a store.  Rendering only items added to the array
        is essential for performance.
      -->
    </template>

  </template>
  <script>
    Polymer({
      created: function() {
        // this list will grow as the user asks for more data.
        // I want the list to be preserved until the user leaves
        // this 'page' to go view something else.
        this.items = [];
      },
      // binding to the count ensures we make requests at appropriate times
      // This will work on on a first render && any updates that happen via
      // user interactions.  
      countChanged: function(oldVal, newVal) {
        if(newVal && Number.isInteger(newVal)) {
          this.makeRequest(newVal);
        }
      },
      // the core-ajax isn't auto in this instance, perhaps I have some 
      // other params or headers to tinker with before i let it fire...
      makeRequest: function(newVal) {
        // min, max, start, end... various components could be added here.
        this.url = 'api/path/to/the/resource?limit='+newVal;
        // once i've done my work, ill have the core-ajax fire away.
        this.$.request.go();
      },
      handleResponse: function(evt, detail, sender) {
        // new data is the detail, i want to append the new items 
        // and preserve what I already have.
        this.addItems(detail);
      },
      additems: function(items) {
        // code here to add new items 
        // Polymer data-binding on this.items may already be smart enough to 
        // compare the old array & the new array and only render the newly added 
        // data.  Otherwise, I'd write a function to check item id's or another
        // unique attribute & append the new items, ensuring DOM changes are only
        // to render additional items, not items I already had.
      }
    });
  </script>
</polymer-element>

Hope its helpful!

[benjaminapetersen]

Curious how you guys feel on all this & if we've generated any confidence in a good path. I'm interested in helping, but not gonna rush in & start hacking given that the conversation has been going before I got into the mix.

[erikringsmuth]

There's one more related issue https://github.com/erikringsmuth/app-router/issues/36 that might be another way to do this.

Something like this.

<app-route singleton></app-route>

The downside is the router would hold onto references of every view so they can never get garbage collected. Although, sometimes that's the ideal solution.

If we don't do that, maybe the simplest addition would be a single attribute like this to tell it to rebind when it matches the same route multiple times.

<app-route rebindOnMatch></app-route>

[benjaminapetersen]

Interesting... I think I prefer your original thought:

<!-- 
  - still assuming 'reload' is default, as is the current behavior, so if the 
    handle-match attrib is not provided nothing changes 
  - especially like that handle-match is on the app-route, not the app-router itself,
    this would be a really flexible
-->
<app-route handle-match="reload|rebind|no-op"></app-route>

Views not getting garbage collected scares me a little bit, I've been bit by that in the past.

[pdf]

Yeah, handle-match is the best answer I think, not sure if rebind is quite the correct term, since we don't want to replace the whole model. As to the discussion on lifecycle events, most of my logic tends to end up in the property observers, after initial setup. If users have the option to choose, it's no big deal anyway.

[erikringsmuth]

@pdf Thinking just updating the path variables and query parameters but not anything in before-data-binding? That sounds fine to me.

I also just realized I did camel case on other attributes and not hyphenated.

<app-route onDuplicateMatch="reload|updateUrlArgs|dontReload"></app-route>

The naming still sounds weird but I'm fine with this approach.

[pdf]

I think before-data-binding should probably still be processed, but instead of replacing the entire model, just update the individual properties, so that any properties not specifically handled by the route remain in place.

Maybe

<app-route onDuplicateMatch="reload|updateModel|noop"></app-route>

Because noop isn't doing reload, or updateModel.

[benjaminapetersen]

Good with camel case, but perhaps just 'onMatch' or 'handleMatch'. It may not be a strict duplicate (view/123 and view/678 both match view/:id, but have different paths), but still a match.

[pdf]

@benjaminapetersen that suggests this will occur on every match though, like the first one.

[erikringsmuth]

<app-route whenTheUrlChangesButStillMatchesThisRoute="reload|updateModel|noop"></app-route>

:stuck_out_tongue_winking_eye:

[benjaminapetersen]

@pdf, the first one is the only one thats irrelevant, I figured it was still clear enough &but pleasantly terse.

@erikringsmuth +1, win.

[pdf]

@benjaminapetersen it's every match after the first one that we want to deal with differently.

[benjaminapetersen]

@pdf, yup. I figured that was clear enough to not require a lengthy attribute name. If you guys think it needs to be spelled out fully, I'm fine with that.

[erikringsmuth]

A short attribute name is the way to go. Maybe this?

<app-route onUrlChange="reload|updateModel|noop"></app-route>

[pdf]

Sounds good to me

[benjaminapetersen]

Ill throw one more out there, 'patternMatch'?

<!-- 
  Docs/README could clarify if the "first match" still seems ambiguous to you guys.  
  IE:
  The patternMatch attribute defines what the route will do when a match occurs.
  The first match will always load dom. Subsequent matches, however, will 
  follow one of the following:
  - reload will generate new dom to replace the old (this is the default behavior)
  - rebind will use existing dom but bind to the updated data model
  - noop assumes nothing needs to happen
-->
<app-route patternMatch="reload|rebind|noop"></app-route>

But I won't cry too hard if vetoed either. :cry: :stuck_out_tongue:

[erikringsmuth]

Or this...

<app-route onmatch="load|updateModel|noop"></app-route>

[benjaminapetersen]

Nice, "onmatch" feels natural & event oriented. Still like "reload|rebind|noop" for the values... :smiling_imp:

[erikringsmuth]

I'm good with this.

<app-route onmatch="reload|rebind|noop"></app-route>

[benjaminapetersen]

looks nice & clean!

[dencold]

I like it too! One thought: should the attribute be "on-match" to conform with Polymer's style for event handling? They seem to have standardized with a dash for things like "on-tap", "on-hold", etc.

[erikringsmuth]

Polymer uses the on-* attributes to map from an event to a function. It'd be like this.

el.addEventListener('match', function() {
  // do something on match
});

Where we'll never actually trigger a match event or bind it to a function.

[dencold]

Ah, gotcha. That makes sense. Sorry for adding extra confusion ;)

[pdf]

Meh, I still think onMatch and rebind might be misleading. I'm not overly concerned about it though personally.

[erikringsmuth]

At this point I'm fine with either of these.

<app-route onUrlChange="reload|updateModel|noop"></app-route>

and

<app-route onmatch="reload|rebind|noop"></app-route>

:shipit:

[pdf]

Time for an executive decision @erikringsmuth :wink:

[erikringsmuth]

Naming things is so hard! Let's go with this and call it a day!

<app-route onUrlChange="reload|updateModel|noop"></app-route>

[dencold]

It's not one of the two hardest things in CS for nothin' ;) Thanks for pushing this forward @erikringsmuth!

[erikringsmuth]

@pdf Want to take a crack at adding in the attribute checks? Otherwise I'll hopefully get some more time this weekend.

[benjaminapetersen]

@erikringsmuth +1 for finalization. @pdf +1 appreciate the rigor on naming.

I will accept my defeat gracefully. Definitely excited to see this moving forward!

[pdf]

@erikringsmuth I won't get a chance to work on this again until the weekend probably, but I'm happy to take a crack at it if you'd like me to.

[benjaminapetersen]

I just copied in the original pull request code from @pdf into the app I'm currently working on to keep pressing on in my dev, but if there is anything I can contribute to help get the idea we have fleshed out on its way, let me know!

[erikringsmuth]

This is on my eventually list, but if you want to continue work off of this PR and submit another one that'd be cool too.

[pdf]

Sorry guys, this time of year is really busy for me - I didn't get a chance to take another pass at this when I'd hoped to. I won't have any time to look at this for a couple of weeks, but if @erikringsmuth hasn't looked at it by then I'll try to sort it out.

[erikringsmuth]

I pulled in the changes and I'm working on them in another branch https://github.com/erikringsmuth/app-router/compare/update-model-on-url-change. I need to do some more testing, but it's a start.

[erikringsmuth]

I've got the changes in v2.5.0 and documented here https://erikringsmuth.github.io/app-router/#/api#onurlchange. Let me know if you find any issues.

Cheers!