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

Can't instantiate component under CE polyfill (where hostAttributes triggers property change handler) #1673

Closed
JanMiksovsky opened this issue May 29, 2015 · 7 comments
Assignees
Labels

Comments

@JanMiksovsky
Copy link

Polymer 1.0 appears to contain a regression from Polymer 0.9. This gist shows a component that uses hostAttributes to set a property; setting the property triggers a property changed handler.

This element cannot be instantiated under Shady DOM. It raises an exception:

TypeError: undefined is not an object (evaluating 'this.__data__[property]')

It looks like a low-level function called _propertySet is trying to access a property bag called data which has not yet been created.

Code such as this used to work up to Polymer 0.9.

This is probably a small bug somewhere, but it's currently preventing Basic Web Components like basic-list-box and basic-carousel from working under Polymer 1.0. We'd obviously like to be able to have those components work with the final 1.0 release.

@kevinpschaaf
Copy link
Member

We'll take a look at fixing this.

However, it's an odd construction to set an attribute on yourself to initialize your own property, and we'd probably consider this an anti-pattern; is there a reason you don't just use value: 'foo' for the target property?

We generally try to avoid using attributes for anything dynamic, and only for initial configuration in markup, since dynamic attribute manipulation is detrimental to performance

@JanMiksovsky
Copy link
Author

@kevinpschaaf The sample code here is intentionally short to make the bug easier to see; the actual use case is slightly more complex.

We have a Polymer behavior that defines an attribute like target here. Components that mix in this behavior want to be able to set that attribute on themselves. A concise and convenient way to do that is via hostAttributes. The component can declaratively set that value instead of having to write, e.g., a ready handler that sets the value imperatively. This effectively falls under the "initial configuration" case you describe above, albeit in JS instead of markup.

So, while the result is that a component is ultimately setting an attribute on itself, it feels pretty different, because the attribute was defined separately in the behavior. We think this is a use case worth supporting.

@kevinpschaaf
Copy link
Member

Components that mix in this behavior want to be able to set that attribute on themselves.

Ok, but is there a reason it is important to think about target as an attribute, rather than a property? Again, we basically only ever think about API in terms of properties, with attributes just as a way to set initial property values via static markup. Behaviors support filling in default property values of properties defined on the base element, ala http://jsbin.com/migozeqowo/2/edit, and this is preferred over attributes because it avoids a needless serialization/deserialization round-trip (and the non-negligible costs of setAttribute/getAttribute).

@JanMiksovsky
Copy link
Author

First: that's an interesting jsBin. I hadn't known that a property could be defined in both a behavior and the component. That's cool! But also, as far as I can tell, undocumented.

Second: Your jsBin example somewhat flips around what I'd described. We have a behavior that defines an attribute (including a changed handler), and the component wants to set that. Your solution has the component define the attribute, and the behavior provides a default value, which is somewhat the opposite. That said, your solution works for the particular use case that motivated us filing this issue.

While I think we're unblocked for the moment, we'd still like to see this fixed. To answer your question, yes, there are reasons for using attributes instead of properties: the former can be referenced in styles.

As a case in point, we have another behavior called Generic that defines a generic attribute on a component mixing in that behavior. The intention of the Generic behavior is to allow the construction of styles that can easily be added/removed by setting/clearing that generic attribute. A component mixing in this attribute might very well want to set this attribute on initialization, and hostAttributes seems like the clearest and most efficient place to do that.

@kevinpschaaf
Copy link
Member

Thanks for bearing with me-- glad you're unblocked. Understanding the use case and available workarounds helps set priority. We took a quick look today and it is related to differences in polyfill timing of attributeChanged when nested inside createdCallback -- the fix will likely be in the polyfill.

@kevinpschaaf
Copy link
Member

Filed webcomponents/webcomponentsjs#325 on webcomponents.js.

The issue can probably be worked around by reordering _marshalHostAttributes and _marshalInstanceEffects in _initFeatures for the short-term.

@kevinpschaaf kevinpschaaf changed the title Can't instantiate component under Shady DOM (where hostAttributes triggers property change handler) Can't instantiate component under CE polyfill (where hostAttributes triggers property change handler) Jun 2, 2015
@kevinpschaaf kevinpschaaf added the p2 label Jun 3, 2015
@kevinpschaaf kevinpschaaf self-assigned this Jun 3, 2015
@JanMiksovsky
Copy link
Author

Thanks for the update.

@sorvell sorvell added p1 and removed p2 labels Aug 3, 2015
@sorvell sorvell assigned sorvell and unassigned kevinpschaaf Aug 3, 2015
@sorvell sorvell closed this as completed in 7c83df5 Aug 4, 2015
kevinpschaaf added a commit that referenced this issue Aug 4, 2015
Fixes #1673: ensure instance effects exist before marshaling attributes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants