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.

Sign up for GitHub

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

Jump to bottom

Add property reflection to notify path and friends calls #5495

Merged
merged 3 commits into from
Mar 6, 2019
Merged

Add property reflection to notify path and friends calls #5495

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad05f56
Add property reflection to notify path and friends calls to support c…
ChadKillingsworth Feb 20, 2019
f13dd75
Fix getStyle definition
kevinpschaaf Mar 6, 2019
03aec68
Revert to `getStyle()`
kevinpschaaf Mar 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/elements/custom-style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export class CustomStyle extends HTMLElement {
* @export
* @return {HTMLStyleElement} This element's light-DOM `<style>`
*/
'getStyle'() {
['getStyle']() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why do we need this at all? What's wrong wrong with getStyle() and the @export we already have?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, export should be sufficient. Will revert back to getStyle()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@export requires both closure-library and the --generate_exports flag to be used with the compiler making it a not-great solution.

There are two options:

  1. Remove the @export annotation and quote the property directly.
  2. Manually export the method with a CustomStyle.prototype['getStyle'] = CustomStyle.prototype.getStyle statement right below the class definition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of our internal users have --generate_exports enabled. We're also advocating for it to become enabled by default. So I think that's our preferred solution. Any reason you don't want to turn on the flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it requires closure library. That's a non starter for most external projects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, seems odd. I thought it was just a JS compiler pass. Maybe it's like JSCompiler_renameProperty and just requires any implementation of the functions that it generates, since the compiler replaces the call anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very odd. The compiler pass literally adds goog.exportSymbol calls but requires the definition of that method to be included as source. Unlike property reflection, the source actually is present in the final output.

The definition for that method could be injected like a polyfill, but nobody has done that work.

if (this._style) {
return this._style;
}
Expand Down