erikringsmuth / app-router

Router for Web Components
https://erikringsmuth.github.io/app-router/
MIT License
611 stars 83 forks source link

Add Events and Ability to defer routes #7

Closed ChrisMcKenzie closed 10 years ago

ChrisMcKenzie commented 10 years ago

It would be really awesome to have events for things like routeChangeStart and routeChangeEnd as well as the ability to defer route until a certain action has been resolved, much like AngularJS router

erikringsmuth commented 10 years ago

Makes sense. I think this would also help add support for transitions (fade in/out, etc.).

Just gathering info at this point. https://github.com/angular-ui/ui-router/wiki#state-change-events https://github.com/flatiron/director#routing-events

I like the idea of before and after. Some routers have on but I'm not sure what the difference between on and after would be.

Should the callbacks be per-route or on the router itself? Maybe both?

erikringsmuth commented 10 years ago

A few more thoughts... The fact that the router has both async and sync loads will make an after callback difficult to implement. Inline templates are synchronous. Imported templates are async with a link.onload() callback. Imported custom elements actually replace the active element immediately (sync), but the element may be unresolved until the HTML import finishes and registerElement() is called. The entire operation is technically async.

It would be fairly straight-forward to implement an on callback that is only called before. An after callback would be tricky especially when importing custom elements.

ChrisMcKenzie commented 10 years ago

Great Thoughts I am going to probably have a go at this tonight. I do like before and after better than on as well.

As for the callbacks I think both would be a nice addition!

erikringsmuth commented 10 years ago

Sounds good. I think these are the potential spots to fire events.

begining of go(), this would be like before https://github.com/erikringsmuth/app-router/blob/master/src/app-router.js#L64

end of go() if no route found https://github.com/erikringsmuth/app-router/blob/master/src/app-router.js#L72

begining of activateRoute() which implies a route was found, this would happen before import operations https://github.com/erikringsmuth/app-router/blob/master/src/app-router.js#L76

begining of replaceActiveElement(), after import but before swapping the active element https://github.com/erikringsmuth/app-router/blob/master/src/app-router.js#L168

end of replaceActiveElement(), after the active element has been replaced with the new element. This won't necessarily mean the element is rendered as it might be an unresolved custom element until the HTML import finishes. https://github.com/erikringsmuth/app-router/blob/master/src/app-router.js#L172

The last place would be to figure out a way to fire an event when the element is done being rendered.

What would be the most useful? Technically they could all be implemented. Another pattern is router.on('eventname', function eventHandler() {}) and router.off('eventName', eventHandler). I Implemented this for another router here https://github.com/erikringsmuth/requirejs-router/blob/master/router.js#L105.

With all of these I don't want to clutter the API, but I want the useful ones available. Maybe just before(callback) at the beginning of go() would be good enough.

erikringsmuth commented 10 years ago

I did some more reading and playing around with events last night. Now I'm leaning toward the native addEventListener, dispatchEvent, and CustomEvent. These are all synchronous and can be canceled with event.preventDefault(). This returns false instead of true and we could use that to cancel the route change operations.

MDN events https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Creating_and_triggering_events

Polymer uses fire(inType, inDetail, inToNode) which they say is a wrapper around dispatchEvent and CustomEvent. http://www.polymer-project.org/docs/polymer/polymer.html#fire

I'm curious if dispatching a CustomEvent will trigger a polymer on-*="{{handler}}". I haven't tried this yet. http://www.polymer-project.org/articles/communication.html#events

I played around with this a little in jsbin. I think we'd actually have to use the old IE9/10 way but we could wrap it in a fire method like Polymer. http://jsbin.com/nezeki/3/edit?html,js,console

naming https://github.com/GoogleWebComponents/style-guide#events http://stackoverflow.com/questions/11450397/javascript-event-naming-conventions

I think all lowercase or hyphenated is best for event names. state-change, not-found, activate-route-start, activate-route-end

Still just a bunch of thoughts at this point. Would you want to try making a router.fire method that wraps a custom event?

ChrisMcKenzie commented 10 years ago

It's funny you say this I have been fighting with myself going that direction as well. I like those event names much better too! I will implement that straight away. :smile:

It would be very nice to be able to use the on-*="{{handler}}" syntax in polymer as well gonna do some R&D and revise my PR

erikringsmuth commented 10 years ago

Awesome! Thanks Chris!

ChrisMcKenzie commented 10 years ago

Just a quick thought here, should we prefix these event with app-router- or just the event name?

erikringsmuth commented 10 years ago

I'm a little unsure how Polymer events propagate. http://www.polymer-project.org/articles/communication.html#events

It looks like Polymer lets all of their events bubble outward from the element and be handled by multiple other outer elements until it hits the document or event.stopEventPropagation() is called. I don't know if event bubbling is a valid use case for a router. We might want to prevent bubbling and force the user to listen by calling router.addEventListener(). In that case I don't think we need to prefix event names. In the other case, if we let events bubble, we should prefix them.

erikringsmuth commented 10 years ago

I played around with events some more last night and added in state-change, not-found, activate-route-start, and activate-route-end events. These are all using native CustomEvents. I added a "lifecycle events" section to the documentation. http://erikringsmuth.github.io/app-router/#/api

This is the main part of the change, but there are a few more commits related to tests. https://github.com/erikringsmuth/app-router/commit/75a2e6aec9dc2e7995b72618114182db1e45b985

ChrisMcKenzie commented 10 years ago

Nice I had a similar commit but I have been out camping and just got back.

Is there a particular reason you wen't with document.createEvent over new CustomEvent(), I feel that the latter is probably a better choice as it is less likely to get deprecated by browsers. Just a thought.

here is my fire method:

router.fire = function(name, data){
    var event = new CustomEvent(name, {
      // From https://github.com/Polymer/core-signals/blob/master/core-signals.html#L60
      // if signals bubble, it's easy to get confusing duplicates
      // (1) listen on a container on behalf of local child
      // (2) some deep child ignores the event and it bubbles
      //     up to said container
      // (3) local child event bubbles up to container
      // also, for performance, we avoid signals flying up the
      // tree from all over the place
      bubbles: false,
      detail: data
    });
    this.dispatchEvent(event);
  };
erikringsmuth commented 10 years ago

Unfortunately the CustomEvent constructor https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent wasn't added till DOM4 which is still a work in progress and didn't have IE support until IE11.

Doing it the DOM3 way document.createEvent('CustomEvent') does exactly the same thing, it's just not as succinct. I think this is what Polymer did too. I'm actually really glad the platform polyfill only goes back to IE9 because IE8 support is a nightmare! ;)

ChrisMcKenzie commented 10 years ago

make sense, cool!