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

Add property reflection to notify path and friends calls #5495

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
28 changes: 17 additions & 11 deletions lib/elements/array-selector.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ let ArraySelectorMixin = dedupingMixin(superClass => {

__updateSelection(multi, itemsInfo) {
let path = itemsInfo.path;
if (path == 'items') {
if (path == JSCompiler_renameProperty('items', this)) {
// Case 1 - items array changed, so diff against previous array and
// deselect any removed items and adjust selected indices
let newItems = itemsInfo.base || [];
Expand All @@ -122,14 +122,14 @@ let ArraySelectorMixin = dedupingMixin(superClass => {
}
this.__lastItems = newItems;
this.__lastMulti = multi;
} else if (itemsInfo.path == 'items.splices') {
} else if (itemsInfo.path == `${JSCompiler_renameProperty('items', this)}.splices`) {
// Case 2 - got specific splice information describing the array mutation:
// deselect any removed items and adjust selected indices
this.__applySplices(itemsInfo.value.indexSplices);
} else {
// Case 3 - an array element was changed, so deselect the previous
// item for that index if it was previously selected
let part = path.slice('items.'.length);
let part = path.slice(`${JSCompiler_renameProperty('items', this)}.`.length);
let idx = parseInt(part, 10);
if ((part.indexOf('.') < 0) && part == idx) {
this.__deselectChangedIdx(idx);
Expand Down Expand Up @@ -167,7 +167,7 @@ let ArraySelectorMixin = dedupingMixin(superClass => {
selected.forEach((idx, item) => {
if (idx < 0) {
if (this.multi) {
this.splice('selected', sidx, 1);
this.splice(JSCompiler_renameProperty('selected', this), sidx, 1);
} else {
this.selected = this.selectedItem = null;
}
Expand All @@ -184,13 +184,19 @@ let ArraySelectorMixin = dedupingMixin(superClass => {
let sidx = 0;
this.__selectedMap.forEach(idx => {
if (idx >= 0) {
this.linkPaths('items.' + idx, 'selected.' + sidx++);
this.linkPaths(
`${JSCompiler_renameProperty('items', this)}.${idx}`,
`${JSCompiler_renameProperty('selected', this)}.${sidx++}`);
}
});
} else {
this.__selectedMap.forEach(idx => {
this.linkPaths('selected', 'items.' + idx);
this.linkPaths('selectedItem', 'items.' + idx);
this.linkPaths(
JSCompiler_renameProperty('selected', this),
`${JSCompiler_renameProperty('items', this)}.${idx}`);
this.linkPaths(
JSCompiler_renameProperty('selectedItem', this),
`${JSCompiler_renameProperty('items', this)}.${idx}`);
});
}
}
Expand Down Expand Up @@ -248,9 +254,9 @@ let ArraySelectorMixin = dedupingMixin(superClass => {
}

__selectedIndexForItemIndex(idx) {
let selected = this.__dataLinkedPaths['items.' + idx];
let selected = this.__dataLinkedPaths[`${JSCompiler_renameProperty('items', this)}.${idx}`];
if (selected) {
return parseInt(selected.slice('selected.'.length), 10);
return parseInt(selected.slice(`${JSCompiler_renameProperty('selected', this)}.`.length), 10);
}
}

Expand All @@ -271,7 +277,7 @@ let ArraySelectorMixin = dedupingMixin(superClass => {
}
this.__updateLinks();
if (this.multi) {
this.splice('selected', sidx, 1);
this.splice(JSCompiler_renameProperty('selected', this), sidx, 1);
} else {
this.selected = this.selectedItem = null;
}
Expand Down Expand Up @@ -318,7 +324,7 @@ let ArraySelectorMixin = dedupingMixin(superClass => {
this.__selectedMap.set(item, idx);
this.__updateLinks();
if (this.multi) {
this.push('selected', item);
this.push(JSCompiler_renameProperty('selected', this), item);
} else {
this.selected = this.selectedItem = item;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/elements/custom-style.js
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
2 changes: 1 addition & 1 deletion lib/elements/dom-repeat.js
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ export class DomRepeat extends domRepeatBase {
if (prop == this.as) {
this.items[idx] = value;
}
let path = translate(this.as, 'items.' + idx, prop);
let path = translate(this.as, `${JSCompiler_renameProperty('items', this)}.${idx}`, prop);
this.notifyPath(path, value);
}
}
Expand Down