From 39c4fcbe77c56d25e78ec34276aa7f8394d2745d Mon Sep 17 00:00:00 2001 From: Nicholas Hyatt Date: Fri, 2 Jul 2021 07:56:10 -0500 Subject: [PATCH 1/7] fix: fix an issue in the custom elements bundle where if you set a property on an element programatically before it was upgraded it would shadow the accessors and therefore break any watchers as well. See https://developers.google.com/web/fundamentals/web-components/best-practices\#lazy-properties --- src/runtime/connected-callback.ts | 2 +- src/runtime/proxy-component.ts | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/runtime/connected-callback.ts b/src/runtime/connected-callback.ts index 235213fa9d0..715383c9395 100644 --- a/src/runtime/connected-callback.ts +++ b/src/runtime/connected-callback.ts @@ -68,7 +68,7 @@ export const connectedCallback = (elm: d.HostElement) => { // Lazy properties // https://developers.google.com/web/fundamentals/web-components/best-practices#lazy-properties - if (BUILD.prop && BUILD.lazyLoad && !BUILD.hydrateServerSide && cmpMeta.$members$) { + if (BUILD.prop && !BUILD.hydrateServerSide && cmpMeta.$members$) { Object.entries(cmpMeta.$members$).map(([memberName, [memberFlags]]) => { if (memberFlags & MEMBER_FLAGS.Prop && elm.hasOwnProperty(memberName)) { const value = (elm as any)[memberName]; diff --git a/src/runtime/proxy-component.ts b/src/runtime/proxy-component.ts index d3e276d7dd7..10be32d3751 100644 --- a/src/runtime/proxy-component.ts +++ b/src/runtime/proxy-component.ts @@ -62,6 +62,21 @@ export const proxyComponent = (Cstr: d.ComponentConstructor, cmpMeta: d.Componen prototype.attributeChangedCallback = function (attrName: string, _oldValue: string, newValue: string) { plt.jmp(() => { const propName = attrNameToPropName.get(attrName); + + // the attr changed callback runs prior to the connected callback where we have logic that also + // has similar logic in it to support lazy properties so if we attempt to unshadow in both places + // to cover the case where an attr was set inline on the non-upgrade element and then the property + // was programatically set which will be handled here or the case where the attr was not set in + // which case the connectedCallback will catch and unshadow. + // https://developers.google.com/web/fundamentals/web-components/best-practices#lazy-properties + // we should think about whether or not we actually want to be reflecting the attributes to properties + // here given that this goes against best practices outlined here + // https://developers.google.com/web/fundamentals/web-components/best-practices#avoid-reentrancy + if (this.hasOwnProperty(propName)) { + newValue = this[propName]; + delete this[propName]; + } + this[propName] = newValue === null && typeof this[propName] === 'boolean' ? false : newValue; }); }; From 0ee6eb6c8f1184ca648cf4b949bbd6c310f8e073 Mon Sep 17 00:00:00 2001 From: Nicholas Hyatt Date: Wed, 7 Jul 2021 10:08:03 -0500 Subject: [PATCH 2/7] update comment with more clarity Co-authored-by: Ryan Waskiewicz --- src/runtime/proxy-component.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/runtime/proxy-component.ts b/src/runtime/proxy-component.ts index 10be32d3751..db22d08276d 100644 --- a/src/runtime/proxy-component.ts +++ b/src/runtime/proxy-component.ts @@ -63,8 +63,8 @@ export const proxyComponent = (Cstr: d.ComponentConstructor, cmpMeta: d.Componen plt.jmp(() => { const propName = attrNameToPropName.get(attrName); - // the attr changed callback runs prior to the connected callback where we have logic that also - // has similar logic in it to support lazy properties so if we attempt to unshadow in both places + // the attr changed callback runs prior to Stencil's connectedCallback, which may also attempt to + // unshadow lazy properties. If we attempt to unshadow in both places // to cover the case where an attr was set inline on the non-upgrade element and then the property // was programatically set which will be handled here or the case where the attr was not set in // which case the connectedCallback will catch and unshadow. From 24cac5b7e51767c16f5d68c371ca81a1c4afeca7 Mon Sep 17 00:00:00 2001 From: Nicholas Hyatt Date: Wed, 7 Jul 2021 10:08:22 -0500 Subject: [PATCH 3/7] update comment with more clarity part 2 Co-authored-by: Ryan Waskiewicz --- src/runtime/proxy-component.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/proxy-component.ts b/src/runtime/proxy-component.ts index db22d08276d..f51b1fd9a85 100644 --- a/src/runtime/proxy-component.ts +++ b/src/runtime/proxy-component.ts @@ -65,7 +65,7 @@ export const proxyComponent = (Cstr: d.ComponentConstructor, cmpMeta: d.Componen // the attr changed callback runs prior to Stencil's connectedCallback, which may also attempt to // unshadow lazy properties. If we attempt to unshadow in both places - // to cover the case where an attr was set inline on the non-upgrade element and then the property + // to cover the case where an attr was set inline on the non-upgraded element and then the property // was programatically set which will be handled here or the case where the attr was not set in // which case the connectedCallback will catch and unshadow. // https://developers.google.com/web/fundamentals/web-components/best-practices#lazy-properties From 1f61bd9b2ab4a5a458ccbf609365332a06c2abd9 Mon Sep 17 00:00:00 2001 From: Nicholas Hyatt Date: Wed, 7 Jul 2021 10:08:48 -0500 Subject: [PATCH 4/7] update comment typo Co-authored-by: Ryan Waskiewicz --- src/runtime/proxy-component.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/proxy-component.ts b/src/runtime/proxy-component.ts index f51b1fd9a85..877fc9f7437 100644 --- a/src/runtime/proxy-component.ts +++ b/src/runtime/proxy-component.ts @@ -66,7 +66,7 @@ export const proxyComponent = (Cstr: d.ComponentConstructor, cmpMeta: d.Componen // the attr changed callback runs prior to Stencil's connectedCallback, which may also attempt to // unshadow lazy properties. If we attempt to unshadow in both places // to cover the case where an attr was set inline on the non-upgraded element and then the property - // was programatically set which will be handled here or the case where the attr was not set in + // was programmatically set which will be handled here or the case where the attr was not set in // which case the connectedCallback will catch and unshadow. // https://developers.google.com/web/fundamentals/web-components/best-practices#lazy-properties // we should think about whether or not we actually want to be reflecting the attributes to properties From 00508a97ecf93f8b9502e945cf4ff593fdc39f7d Mon Sep 17 00:00:00 2001 From: Nicholas Hyatt Date: Wed, 7 Jul 2021 11:18:52 -0500 Subject: [PATCH 5/7] another take on the comment :-) --- src/runtime/proxy-component.ts | 42 ++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/src/runtime/proxy-component.ts b/src/runtime/proxy-component.ts index 877fc9f7437..c994e3c9e97 100644 --- a/src/runtime/proxy-component.ts +++ b/src/runtime/proxy-component.ts @@ -63,15 +63,39 @@ export const proxyComponent = (Cstr: d.ComponentConstructor, cmpMeta: d.Componen plt.jmp(() => { const propName = attrNameToPropName.get(attrName); - // the attr changed callback runs prior to Stencil's connectedCallback, which may also attempt to - // unshadow lazy properties. If we attempt to unshadow in both places - // to cover the case where an attr was set inline on the non-upgraded element and then the property - // was programmatically set which will be handled here or the case where the attr was not set in - // which case the connectedCallback will catch and unshadow. - // https://developers.google.com/web/fundamentals/web-components/best-practices#lazy-properties - // we should think about whether or not we actually want to be reflecting the attributes to properties - // here given that this goes against best practices outlined here - // https://developers.google.com/web/fundamentals/web-components/best-practices#avoid-reentrancy + // In a webcomponent lifecyle the attributeChangedCallback runs prior to connectedCallback + // in the case where an attribute was set inline. + // ```html + // + // ``` + // + // There is an edge case where a developer sets the attribute inline on a custom element and then programatically + // changes it before it has been upgraded as shown below: + // + // ```html + // + // + // + // ``` + // In this case if we do not unshadow here and use the value of the shadowing property attributeChangedCallback + // will be called with `newValue = "some-value"` and will set the shadowed property (this.someAttribute = "another-value" + // to the value that was set inline i.e. "some-value" from above example and when + // the connectedCallback attempts to unshadow it will use "some-value" as the intial value rather than "another-value" + // + // The case where the attribute was NOT set inline but was not set programmatically shall be handled/unshadowed + // by connectedCallback as this attributeChangedCallback will not fire. + // + // https://developers.google.com/web/fundamentals/web-components/best-practices#lazy-properties + // + // TODO(STENCIL-16) we should think about whether or not we actually want to be reflecting the attributes to + // properties here given that this goes against best practices outlined here + // https://developers.google.com/web/fundamentals/web-components/best-practices#avoid-reentrancy if (this.hasOwnProperty(propName)) { newValue = this[propName]; delete this[propName]; From 8715f36f97aabc774b787bfc65a0bf787924ec25 Mon Sep 17 00:00:00 2001 From: Nicholas Hyatt Date: Wed, 7 Jul 2021 11:32:50 -0500 Subject: [PATCH 6/7] Update src/runtime/proxy-component.ts Co-authored-by: Ryan Waskiewicz --- src/runtime/proxy-component.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/runtime/proxy-component.ts b/src/runtime/proxy-component.ts index c994e3c9e97..439d875c038 100644 --- a/src/runtime/proxy-component.ts +++ b/src/runtime/proxy-component.ts @@ -83,8 +83,8 @@ export const proxyComponent = (Cstr: d.ComponentConstructor, cmpMeta: d.Componen // cutsomElements.define('my-component', MyComponent); // // ``` - // In this case if we do not unshadow here and use the value of the shadowing property attributeChangedCallback - // will be called with `newValue = "some-value"` and will set the shadowed property (this.someAttribute = "another-value" + // In this case if we do not unshadow here and use the value of the shadowing property, attributeChangedCallback + // will be called with `newValue = "some-value"` and will set the shadowed property (this.someAttribute = "another-value") // to the value that was set inline i.e. "some-value" from above example and when // the connectedCallback attempts to unshadow it will use "some-value" as the intial value rather than "another-value" // From f38e544fa7836512e9fc892dd0de4c1b5e4640ac Mon Sep 17 00:00:00 2001 From: Nicholas Hyatt Date: Wed, 7 Jul 2021 11:32:56 -0500 Subject: [PATCH 7/7] Update src/runtime/proxy-component.ts Co-authored-by: Ryan Waskiewicz --- src/runtime/proxy-component.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/proxy-component.ts b/src/runtime/proxy-component.ts index 439d875c038..f3b8f95a524 100644 --- a/src/runtime/proxy-component.ts +++ b/src/runtime/proxy-component.ts @@ -85,7 +85,7 @@ export const proxyComponent = (Cstr: d.ComponentConstructor, cmpMeta: d.Componen // ``` // In this case if we do not unshadow here and use the value of the shadowing property, attributeChangedCallback // will be called with `newValue = "some-value"` and will set the shadowed property (this.someAttribute = "another-value") - // to the value that was set inline i.e. "some-value" from above example and when + // to the value that was set inline i.e. "some-value" from above example. When // the connectedCallback attempts to unshadow it will use "some-value" as the intial value rather than "another-value" // // The case where the attribute was NOT set inline but was not set programmatically shall be handled/unshadowed