erikringsmuth / app-router

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

Allows for inline template binding of polymer elements #41

Closed jamesjnadeau closed 9 years ago

jamesjnadeau commented 9 years ago

Really simple, just expanded upon what was added for #19. Properly filled out the call to template.createInstance.

I was using firebase-element's firebase-login element, and wanted to have a global instance of it to pass around to my different page elements. this way my index page is just calling in one polymer element, and it goes from there.

I updated the readme with an example of what I used this for. Ran into this issue on Saturday and found a solution this morning after tracing through the code and trying to do this any other way imaginable... :)

I would like to allow lazy loading of an element with this variable binding functionality, but this will work great for now.

Thanks for this awesome web component! Happy to make any updates you see necessary.

erikringsmuth commented 9 years ago

Awesome! Do you know where I can find documentation or the source for these?

I just want to read up on it so I know exactly what's going on.

jamesjnadeau commented 9 years ago

I don't, sorry, I just looked at the instances of it in polymer.js file and guessed at the inputs. There's not much doc I could find on this yet.

This one lead me down the right path: https://github.com/Polymer/polymer/blob/87b3815812aaa9f956f044e74d43460b1babee5f/src/instance/mdv.js#L37 and is where I got the "template.bindingDelegate" part.

I got the templateInstance.model idea from https://www.polymer-project.org/docs/polymer/databinding.html#event-handling-and-data-binding

erikringsmuth commented 9 years ago

Alright, I'm going to play around with this over the next week and try some edge cases but beyond that it looks good. This may also solve issue 28 at the same time.

jamesjnadeau commented 9 years ago

Have fun, let me know what you find.

There should probably be some tests for this someway somehow... not sure where to start with that, but if you point me in a direction, would love to look at it and see what I can do to help.

jamesjnadeau commented 9 years ago

@erikringsmuth: I added in some more that I needed so I could do some basic url parameter parsing for these polymer stamped templates.

erikringsmuth commented 9 years ago

I pulled in your changes to branch feature-inline-polymer-template. I'm trying out a few more things before I merge it to master.

As far as I can tell the template.bindingDelegate doesn't do anything. This is the useful part template.createInstance(model).

It looks like Polymer sets a templateInstance property on every DOM node in a Polymer element which means template.templateInstance.model points to the app-main model in your example. This can be generalized further to work with imported templates by referencing router.templateInstance.model. This will always point to the model of the Polymer element that contains the app-router.

One thing I want to avoid is setting properties back to the templateInstance.model like this https://github.com/erikringsmuth/app-router/pull/41/files#diff-8f495b0b37d1611ef1e4046e099aa7e1R210 because it will effectively set those properties on the outer Polymer element too. The workaround I'm trying to use is to shallow copy the templateInstance.model onto a different model object https://github.com/erikringsmuth/app-router/compare/feature-inline-polymer-template#diff-8f495b0b37d1611ef1e4046e099aa7e1R269. The problem is this isn't working... Once I get this figured out I think it's good to go.

jamesjnadeau commented 9 years ago

interesting note about the bindingDelegate, I'll have to look into it more...

I've been looking at this doc: https://www.polymer-project.org/docs/polymer/template.html#activating-a-template

and the more I read it, the more I think this is how it should work in this case, This is a sub-template of a polymer element, so it would naturally share it's parents scope, unless the scope is limited by the templates attributes(which my changes currently don't support).

I currently don't have a problem with the route parameters bubbeling up to the parent polymer element, I think this is a feature, you can do things in your main polymer element with this information.

Perhaps in the polymer import case, the models should be kept separate and bound only upon certain parameters being being present in the app-route element. Thinking out loud, feel free to disagree :)

jamesjnadeau commented 9 years ago

PS just found this example about binding delegates and what they could be used for.

https://github.com/Polymer/TemplateBinding/blob/master/examples/how_to/custom_syntax.html

it would seem to me that not including might remove some possible functionality.

erikringsmuth commented 9 years ago

Yeah, I was just toying with the idea of binding all published attributes from the parent element but it'd probably make more sense to specify a list of parent scope attributes on the app-route. That'd keep it much more limited. Also doing a shallow copy of the templateInstance.model copies a ton of unnecessary DOM properties.

I just pushed a few commits that would copy specific parent scope attributes if you set them like this.

<app-route path="/other/:firstProperty" parentScopeAttributes="secondProperty thirdProperty">

There's a problem though. It only sets the properties once so it doesn't listen for changes later. The only way to pick up a parent property is if it's set as an attribute on the parent element or it's set in the created() method. ready() is called too late.

I've got to stop for tonight but I'll pick it up again tomorrow. This is where I'm leaving off https://github.com/erikringsmuth/app-router/compare/feature-inline-polymer-template.

erikringsmuth commented 9 years ago

Oh, and the bindingDelegate looks like it falls back to something https://github.com/Polymer/TemplateBinding/blob/master/src/TemplateBinding.js#L511. I'm not sure what this is but it's another thing to keep digging into...

erikringsmuth commented 9 years ago

The more I look at this the more I die a little inside :expressionless:

I could do exactly what your initial suggestion was. Extend the parent scope with the route variables and create a template instance with template.createInstance(model).

This creates a document fragment with the contents of the template bound to the model. This is actually pretty sweet because it has 2-way data binding when you create it this way so changes to properties in a child reflects in the parent and vice versa.

The problem I'm running into is that there's no consistency between the way templates work and the way custom elements work. There's no equivalent way to bind an entire parent scope to a custom element. You have to bind attributes and properties 1 at a time. You also can't bind every property from the parent scope because it throws errors on some of the parent scope DOM properties.

The one piece of information I can get about the parent scope is the published attributes. But these aren't all of the relevant properties since not all properties are published.

Finally, even if I specify a list of attributes to bind, it's only 1 time, 1-way binding. If the parent hasn't already set the property (think async ajax responses), then the property won't get set on the child. To properly set up dynamic data binding is going way over my head at this point.

In the end, binding to custom elements feels like it will cause more problems than it will solve. I'm still on the border whether the child modifying the parent scope is a reasonable thing to do for template binding and If I copy the parent scope's properties to a different object I'm back to the 1-time binding dilemma.

I'm going to have to sit and think on this one for a while.

In the mean time, there's potentially another way to achieve the same type of thing with "global state" elements.

auth-global.html

<link rel="import" href="auth-provider.html">

<!-- there will only be one instance of this created because it is outside of the custom
element's template definition -->
<auth-provider id="auth"></auth-provider>

<polymer-element name="auth-global" attributes="auth">
  <template>
  </template>
  <script>
    Polymer({
      ready:function() {
        this.auth = document.querySelector('#auth');
      }
    });
  </script>
</polymer-element>

Then later in app-user.html and app-login.html you could reference the auth-global element to bind to a singleton-like element.

<auth-global auth="{{auth}}"></auth-global>

Creating multiple auth-global elements should only ever create 1 auth-provider element.

erikringsmuth commented 9 years ago

I've got another idea that looks promising! It's on a new template-binding branch.

This includes your changes to bind path variables and query parameters to templates. Then in addition to that I can bind a property called parentModel to templates and custom elements.

If the router is in a Polymer template or custom element parentModel will be a reference to router.templateInstance.model.

This should let you use {{parentModel.exampleProperty}} from within any custom element or template.

I want to spend some more time testing this but it looks like it's working so far. You can pull the code from this branch if you want to try it out.

jamesjnadeau commented 9 years ago

lol, loved the "die a little inside" comment :) feel the same way.

Thanks for the note on the global option, I was trying that first, but didn't get that I could put the element there and it would be global so I was doomed to fail, makes sense now.

The parentModel doesn't sit as well with me. Seems like you're removing the selective nature of the custom elements attribute. So your observing the changes of the entire parentModel, instead of the parts of it you need. At first glance this seems wasteful, but I'm not sure I know enough about what is going on to say I know better than you ;)

I was reading about this yesterday, figured I'd share as I think this is along the same lines of what we are doing here.

https://github.com/polymer/observe-js

here's an example

<polymer-element name="app-menu" attributes="auth">
    <template>
    </template>
Polymer({
    observe: {
        auth: 'authChange'
    },
    authChange: function() {
        console.log('auth was changed');
    }
}); 

This is an element that is the child of the parent app element, aka it's a sibling of the app-router element. I needed to watch a nested child of the auth attribute, and doing authChange on it's own wasn't firing.

I think investigating this observe functionality might lead use to the holy grail we are looking for.....

jamesjnadeau commented 9 years ago

PS here's what I'm working on and testing this out with: https://github.com/jamesjnadeau/twillio_client/blob/master/static/components/app-main.html

I checked looked over the template-binding branch, but didn't try it.

erikringsmuth commented 9 years ago

If you look at the 3rd example here it shows how to observe nested properties https://www.polymer-project.org/docs/polymer/polymer.html#observeblock.

Like this.

observe: {
  'auth.first': 'authFirstChange'
}
erikringsmuth commented 9 years ago

I've continued to change my mind to no end.

I found some info on how Polymer treats parent scopes https://www.polymer-project.org/docs/polymer/expressions.html#nested-scoping-rules. Basically it doesn't allow you to access the parent scope unless you name it.

Now I'm leaning toward adding the ability to bind the router to the template/custom element if the <app-route> has a bindRoute attribute. Then you can use router.templateInstance.model to get the parent scope that contains the router. Every DOM node in a Polymer template has the templateInstance property. https://www.polymer-project.org/docs/polymer/databinding.html#event-handling-and-data-binding

You could set up the router like this.

<polymer-element name="router-binding-example" attributes="publishedAttr">
  <template>
    <app-router>
      <app-route path="/:pathVar" bindRouter>
        <template>
          <h2>parent published attribute - {{router.templateInstance.model.publishedAttr}}</h2>
          <h2>parent property (not published) - {{router.templateInstance.model.elementProperty}}</h2>
          <h2>route arg bound to template - {{pathVar}}</h2>
        </template>
      </app-route>
      <app-route path="*">
        <template>Not Found</template>
      </app-route>
    </app-router>
  </template>
  <script>
    Polymer('router-binding-example', {
      ready: function() {
        this.elementProperty = '2nd arg!';
      }
    });
  </script>
</polymer-element>

<router-binding-example first="1st arg!"></router-binding-example>

Given this URL /123, should produce something like this.

<h2>parent published attribute - 1st arg!</h2>
<h2>parent property (not published) - 2nd arg!</h2>
<h2>route arg bound to template - 123</h2>

Once again, I'm going to think about this for a few days because there's a good chance I'll want to change it again.

jamesjnadeau commented 9 years ago

:) This solution seems like a lot of typing and repetitive use of "router.templateInstance.model" What advantages does it bring?

erikringsmuth commented 9 years ago

I'm going to do the bindRouter option as a first pass. It's a good feature even without the parent scope part because it allows access to the router from within a route.

The biggest thing is I don't have to do dynamic data-binding and the code change is small. I'll still consider binding individual variables if I can figure out a good clean way to do it.

I'll admit I definitely prefer the global/singleton like element over accessing parent scope so that's part of it. This will still allow a user to get to the parent scope if they need to. The downside is it's very verbose.

Binding to templates is going to be another win!

I'll get a 2.1.0 version released in about a week. I want to get this bug fix worked out before I updated it. https://github.com/erikringsmuth/app-router/pull/43

erikringsmuth commented 9 years ago

Another idea... I could add a before-data-binding event.

<app-route ... on-before-data-binding="{{updateRouteModel}}"></app-route>

The event handler

updateRouteModel: function(event) {
  // event.detail looks like this
  {
    path: '/new-path',
    route: newRoute,
    oldRoute: oldRoute,
    model: {
      // path variables and query parameters
      // modify or add properties here before data binding happens
    }
  }
}

Then you could modify the model with any values.

jamesjnadeau commented 9 years ago

I really like that idea!

erikringsmuth commented 9 years ago

Cool! That should only be ~3 lines of code. I'll get that in there and try to get 2.1.0 released over the weekend.

erikringsmuth commented 9 years ago

I got 2.1.0 released with the changes!

I have the before-data-binding event documented here https://erikringsmuth.github.io/app-router/#/events.

And more info on the other data binding features here https://erikringsmuth.github.io/app-router/#/databinding/1337?queryParam1=Routing%20with%20Web%20Components!.

jamesjnadeau commented 9 years ago

Thanks for the updats! :)