Skip to content

Commit 63dbb47

Browse files
authored
fix(runtime): prevent unnecessary re-renders when reflecting props (#3106)
this commit fixes a very specific issue, where reflected props may lead to unnecessary re-renders. in order to trigger the issue, each of the following conditions must be met: 1. the lazy load build of stencil is used 2. a prop in a stencil component is created 2.1. the prop is created with `{reflect: true}` 2.2. the member is a complex type, such as `number | any`. types such as `number` and `number | number` will not trigger the error. using the optional token following the member's name will not create a complex type for the purposes of this bug (e.g. `@Prop() val?: number`) 3. the stencil component containing the prop must have a `render` fn 4. the value passed to the component must not be a string. this implies that the component cannot be rendered directly within an html file, as the prop will be converted to a DOMString implicitly when the conditions above are met, the problem will manifest itself as a warning in the console when running in developer mode: ``` The state/prop "PROP_NAME" changed during rendering. This can potentially lead to infinite-loops and other bugs" Element <my-component val="1" class="hydrated>...</my-component> New value 1 Old value 1 ``` what is happening under the hood is the component containing the prop will be rendered by stencil. stencil will then attempt to reflect the prop back to the attribute on the component's element via a call to `setAttribute(elm, val)`. `setAttribute` implicitly converts the val provided as the second argument to a DOMString, which is then converted to a JavaScript string. invoking `setAttribute` initiates `attributeChangedCallback` being called on the component, using the aforementioned string value. `attributeChangedCallback` leads to a series of `set*` calls internally before using the string representation of the prop, even when a number is passed like so: ```tsx <my-component my-prop={1}></my-component> ``` the reason for this is that when doing change detection on prop values, stencil performs strict equality matching on the new value against an older known value (outside the context of `attributeChangedCallback`) using the `===` operator. with this change, we attempt to detect a case of accidental type coercion to a DOMString for the same value. within `attributeChangedCallback`, the prop may be looked up on the prototype of the constructor of the proxy component. If the prop is found on the prototype, is of type number, and the string received matches the number, the change is discarded. attempts to match the string against other primitives such as boolean, null, and undefined have been intentionally avoided for this commit for simplicity: - boolean attributes are considered `true` if they are present and their value is the emtpy string or the attribute's name - null is traditionally sets the value to the string 'null', rather than nullifying or removing the value this commit fixes up small issues that are inconsequential to the implementation of this fix, but were found during the debugging process. they are listed below: - removing an unused tslint:disable pragma - fixing typos - adding curly braces where needed - simplifying a ternary expression
1 parent 9039c23 commit 63dbb47

13 files changed

Lines changed: 144 additions & 17 deletions

File tree

src/compiler/transformers/decorators-to-static/convert-decorators.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,9 @@ export const visitClassDeclaration = (
5454
// parser component decorator (Component)
5555
componentDecoratorToStatic(config, typeChecker, diagnostics, classNode, newMembers, componentDecorator);
5656

57-
// parse member decorators (Prop, State, Listen, Event, Method, Element and Watch)
57+
// stores a reference to fields that should be watched for changes
5858
const watchable = new Set<string>();
59+
// parse member decorators (Prop, State, Listen, Event, Method, Element and Watch)
5960
if (decoratedMembers.length > 0) {
6061
propDecoratorsToStatic(diagnostics, decoratedMembers, typeChecker, watchable, newMembers);
6162
stateDecoratorsToStatic(decoratedMembers, watchable, newMembers);

src/compiler/transformers/decorators-to-static/prop-decorator.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,8 @@ const parsePropDecorator = (
4242
return null;
4343
}
4444

45-
const decoratorParms = getDeclarationParameters<d.PropOptions>(propDecorator);
46-
const propOptions: d.PropOptions = decoratorParms[0] || {};
45+
const decoratorParams = getDeclarationParameters<d.PropOptions>(propDecorator);
46+
const propOptions: d.PropOptions = decoratorParams[0] || {};
4747

4848
const propName = prop.name.getText();
4949

src/compiler/transformers/decorators-to-static/watch-decorator.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,10 @@ const parseWatchDecorator = (
3232
return method.decorators.filter(isDecoratorNamed('Watch')).map((decorator) => {
3333
const [propName] = getDeclarationParameters<string>(decorator);
3434
if (!watchable.has(propName)) {
35-
const dianostic = config.devMode ? buildWarn(diagnostics) : buildError(diagnostics);
36-
dianostic.messageText = `@Watch('${propName}') is trying to watch for changes in a property that does not exist.
35+
const diagnostic = config.devMode ? buildWarn(diagnostics) : buildError(diagnostics);
36+
diagnostic.messageText = `@Watch('${propName}') is trying to watch for changes in a property that does not exist.
3737
Make sure only properties decorated with @State() or @Prop() are watched.`;
38-
augmentDiagnosticWithNode(dianostic, decorator);
38+
augmentDiagnosticWithNode(diagnostic, decorator);
3939
}
4040
return {
4141
propName,

src/compiler/transformers/static-to-meta/props.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ export const parseStaticProps = (staticMembers: ts.ClassElement[]): d.ComponentC
1919
name: propName,
2020
type: val.type,
2121
attribute: val.attribute ? val.attribute.toLowerCase() : undefined,
22-
reflect: typeof val.reflect === 'boolean' ? val.reflect : typeof val.reflect === 'boolean' ? val.reflect : false,
22+
reflect: typeof val.reflect === 'boolean' ? val.reflect : false,
2323
mutable: !!val.mutable,
2424
required: !!val.required,
2525
optional: !!val.optional,

src/runtime/bootstrap-lazy.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ export const bootstrapLazy = (lazyBundles: d.LazyBundlesRuntimeData, options: d.
4949
}
5050
}
5151

52-
lazyBundles.map((lazyBundle) =>
52+
lazyBundles.map((lazyBundle) => {
5353
lazyBundle[1].map((compactMeta) => {
5454
const cmpMeta: d.ComponentRuntimeMeta = {
5555
$flags$: compactMeta[0],
@@ -159,8 +159,8 @@ export const bootstrapLazy = (lazyBundles: d.LazyBundlesRuntimeData, options: d.
159159
proxyComponent(HostElement as any, cmpMeta, PROXY_FLAGS.isElementConstructor) as any
160160
);
161161
}
162-
})
163-
);
162+
});
163+
});
164164

165165
if (BUILD.invisiblePrehydration && (BUILD.hydratedClass || BUILD.hydratedAttribute)) {
166166
visibilityStyle.innerHTML = cmpTags + HYDRATED_CSS;

src/runtime/initialize-component.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ export const initializeComponent = async (
121121
const schedule = () => scheduleUpdate(hostRef, true);
122122

123123
if (BUILD.asyncLoading && ancestorComponent && ancestorComponent['s-rc']) {
124-
// this is the intial load and this component it has an ancestor component
124+
// this is the initial load and this component it has an ancestor component
125125
// but the ancestor component has NOT fired its will update lifecycle yet
126126
// so let's just cool our jets and wait for the ancestor to continue first
127127

src/runtime/proxy-component.ts

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -74,14 +74,14 @@ export const proxyComponent = (Cstr: d.ComponentConstructor, cmpMeta: d.Componen
7474
plt.jmp(() => {
7575
const propName = attrNameToPropName.get(attrName);
7676

77-
// In a webcomponent lifecyle the attributeChangedCallback runs prior to connectedCallback
77+
// In a web component lifecycle the attributeChangedCallback runs prior to connectedCallback
7878
// in the case where an attribute was set inline.
7979
// ```html
8080
// <my-component some-attribute="some-value"></my-component>
8181
// ```
8282
//
83-
// There is an edge case where a developer sets the attribute inline on a custom element and then programatically
84-
// changes it before it has been upgraded as shown below:
83+
// There is an edge case where a developer sets the attribute inline on a custom element and then
84+
// programmatically changes it before it has been upgraded as shown below:
8585
//
8686
// ```html
8787
// <!-- this component has _not_ been upgraded yet -->
@@ -91,13 +91,13 @@ export const proxyComponent = (Cstr: d.ComponentConstructor, cmpMeta: d.Componen
9191
// el = document.querySelector("#test");
9292
// el.someAttribute = "another-value";
9393
// // upgrade component
94-
// cutsomElements.define('my-component', MyComponent);
94+
// customElements.define('my-component', MyComponent);
9595
// </script>
9696
// ```
9797
// In this case if we do not unshadow here and use the value of the shadowing property, attributeChangedCallback
9898
// will be called with `newValue = "some-value"` and will set the shadowed property (this.someAttribute = "another-value")
9999
// to the value that was set inline i.e. "some-value" from above example. When
100-
// the connectedCallback attempts to unshadow it will use "some-value" as the intial value rather than "another-value"
100+
// the connectedCallback attempts to unshadow it will use "some-value" as the initial value rather than "another-value"
101101
//
102102
// The case where the attribute was NOT set inline but was not set programmatically shall be handled/unshadowed
103103
// by connectedCallback as this attributeChangedCallback will not fire.
@@ -110,6 +110,15 @@ export const proxyComponent = (Cstr: d.ComponentConstructor, cmpMeta: d.Componen
110110
if (this.hasOwnProperty(propName)) {
111111
newValue = this[propName];
112112
delete this[propName];
113+
} else if (
114+
prototype.hasOwnProperty(propName) &&
115+
typeof this[propName] === 'number' &&
116+
this[propName] == newValue
117+
) {
118+
// if the propName exists on the prototype of `Cstr`, this update may be a result of Stencil using native
119+
// APIs to reflect props as attributes. Calls to `setAttribute(someElement, propName)` will result in
120+
// `propName` to be converted to a `DOMString`, which may not be what we want for other primitive props.
121+
return;
113122
}
114123

115124
this[propName] = newValue === null && typeof this[propName] === 'boolean' ? false : newValue;

src/runtime/vdom/set-accessor.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,6 @@ export const setAccessor = (
110110
// Workaround for Safari, moving the <input> caret when re-assigning the same valued
111111
if (memberName === 'list') {
112112
isProp = false;
113-
// tslint:disable-next-line: triple-equals
114113
} else if (oldValue == null || (elm as any)[memberName] != n) {
115114
(elm as any)[memberName] = n;
116115
}

test/karma/test-app/components.d.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,9 @@ export namespace Components {
4747
}
4848
interface BuildData {
4949
}
50+
interface ChildWithReflection {
51+
"val": number | any;
52+
}
5053
interface CmpLabel {
5154
}
5255
interface CmpLabelWithSlotSibling {
@@ -173,6 +176,8 @@ export namespace Components {
173176
}
174177
interface NodeResolution {
175178
}
179+
interface ParentWithReflectChild {
180+
}
176181
interface ReflectToAttr {
177182
"bool": boolean;
178183
"disabled": boolean;
@@ -369,6 +374,12 @@ declare global {
369374
prototype: HTMLBuildDataElement;
370375
new (): HTMLBuildDataElement;
371376
};
377+
interface HTMLChildWithReflectionElement extends Components.ChildWithReflection, HTMLStencilElement {
378+
}
379+
var HTMLChildWithReflectionElement: {
380+
prototype: HTMLChildWithReflectionElement;
381+
new (): HTMLChildWithReflectionElement;
382+
};
372383
interface HTMLCmpLabelElement extends Components.CmpLabel, HTMLStencilElement {
373384
}
374385
var HTMLCmpLabelElement: {
@@ -705,6 +716,12 @@ declare global {
705716
prototype: HTMLNodeResolutionElement;
706717
new (): HTMLNodeResolutionElement;
707718
};
719+
interface HTMLParentWithReflectChildElement extends Components.ParentWithReflectChild, HTMLStencilElement {
720+
}
721+
var HTMLParentWithReflectChildElement: {
722+
prototype: HTMLParentWithReflectChildElement;
723+
new (): HTMLParentWithReflectChildElement;
724+
};
708725
interface HTMLReflectToAttrElement extends Components.ReflectToAttr, HTMLStencilElement {
709726
}
710727
var HTMLReflectToAttrElement: {
@@ -1046,6 +1063,7 @@ declare global {
10461063
"attribute-html-root": HTMLAttributeHtmlRootElement;
10471064
"bad-shared-jsx": HTMLBadSharedJsxElement;
10481065
"build-data": HTMLBuildDataElement;
1066+
"child-with-reflection": HTMLChildWithReflectionElement;
10491067
"cmp-label": HTMLCmpLabelElement;
10501068
"cmp-label-with-slot-sibling": HTMLCmpLabelWithSlotSiblingElement;
10511069
"conditional-basic": HTMLConditionalBasicElement;
@@ -1102,6 +1120,7 @@ declare global {
11021120
"no-delegates-focus": HTMLNoDelegatesFocusElement;
11031121
"node-globals": HTMLNodeGlobalsElement;
11041122
"node-resolution": HTMLNodeResolutionElement;
1123+
"parent-with-reflect-child": HTMLParentWithReflectChildElement;
11051124
"reflect-to-attr": HTMLReflectToAttrElement;
11061125
"reparent-style-no-vars": HTMLReparentStyleNoVarsElement;
11071126
"reparent-style-with-vars": HTMLReparentStyleWithVarsElement;
@@ -1198,6 +1217,9 @@ declare namespace LocalJSX {
11981217
}
11991218
interface BuildData {
12001219
}
1220+
interface ChildWithReflection {
1221+
"val"?: number | any;
1222+
}
12011223
interface CmpLabel {
12021224
}
12031225
interface CmpLabelWithSlotSibling {
@@ -1331,6 +1353,8 @@ declare namespace LocalJSX {
13311353
}
13321354
interface NodeResolution {
13331355
}
1356+
interface ParentWithReflectChild {
1357+
}
13341358
interface ReflectToAttr {
13351359
"bool"?: boolean;
13361360
"disabled"?: boolean;
@@ -1476,6 +1500,7 @@ declare namespace LocalJSX {
14761500
"attribute-html-root": AttributeHtmlRoot;
14771501
"bad-shared-jsx": BadSharedJsx;
14781502
"build-data": BuildData;
1503+
"child-with-reflection": ChildWithReflection;
14791504
"cmp-label": CmpLabel;
14801505
"cmp-label-with-slot-sibling": CmpLabelWithSlotSibling;
14811506
"conditional-basic": ConditionalBasic;
@@ -1532,6 +1557,7 @@ declare namespace LocalJSX {
15321557
"no-delegates-focus": NoDelegatesFocus;
15331558
"node-globals": NodeGlobals;
15341559
"node-resolution": NodeResolution;
1560+
"parent-with-reflect-child": ParentWithReflectChild;
15351561
"reflect-to-attr": ReflectToAttr;
15361562
"reparent-style-no-vars": ReparentStyleNoVars;
15371563
"reparent-style-with-vars": ReparentStyleWithVars;
@@ -1603,6 +1629,7 @@ declare module "@stencil/core" {
16031629
"attribute-html-root": LocalJSX.AttributeHtmlRoot & JSXBase.HTMLAttributes<HTMLAttributeHtmlRootElement>;
16041630
"bad-shared-jsx": LocalJSX.BadSharedJsx & JSXBase.HTMLAttributes<HTMLBadSharedJsxElement>;
16051631
"build-data": LocalJSX.BuildData & JSXBase.HTMLAttributes<HTMLBuildDataElement>;
1632+
"child-with-reflection": LocalJSX.ChildWithReflection & JSXBase.HTMLAttributes<HTMLChildWithReflectionElement>;
16061633
"cmp-label": LocalJSX.CmpLabel & JSXBase.HTMLAttributes<HTMLCmpLabelElement>;
16071634
"cmp-label-with-slot-sibling": LocalJSX.CmpLabelWithSlotSibling & JSXBase.HTMLAttributes<HTMLCmpLabelWithSlotSiblingElement>;
16081635
"conditional-basic": LocalJSX.ConditionalBasic & JSXBase.HTMLAttributes<HTMLConditionalBasicElement>;
@@ -1659,6 +1686,7 @@ declare module "@stencil/core" {
16591686
"no-delegates-focus": LocalJSX.NoDelegatesFocus & JSXBase.HTMLAttributes<HTMLNoDelegatesFocusElement>;
16601687
"node-globals": LocalJSX.NodeGlobals & JSXBase.HTMLAttributes<HTMLNodeGlobalsElement>;
16611688
"node-resolution": LocalJSX.NodeResolution & JSXBase.HTMLAttributes<HTMLNodeResolutionElement>;
1689+
"parent-with-reflect-child": LocalJSX.ParentWithReflectChild & JSXBase.HTMLAttributes<HTMLParentWithReflectChildElement>;
16621690
"reflect-to-attr": LocalJSX.ReflectToAttr & JSXBase.HTMLAttributes<HTMLReflectToAttrElement>;
16631691
"reparent-style-no-vars": LocalJSX.ReparentStyleNoVars & JSXBase.HTMLAttributes<HTMLReparentStyleNoVarsElement>;
16641692
"reparent-style-with-vars": LocalJSX.ReparentStyleWithVars & JSXBase.HTMLAttributes<HTMLReparentStyleWithVarsElement>;
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
import { Component, h, Prop } from '@stencil/core';
2+
3+
@Component({
4+
tag: 'child-with-reflection',
5+
// 'shadow' is not needed here, but does make testing easier by using the shadow root to help encapsulate textContent
6+
shadow: true,
7+
})
8+
export class ChildWithReflection {
9+
// counter to proxy the number of times a render has occurred, since we don't have access to those dev tools during
10+
// karma tests
11+
renderCount = 0;
12+
13+
// to properly replicate the issue:
14+
// - `reflect` must be `true`
15+
// - the type of the prop must be complex, e.g. `number | any`
16+
// - the value passed in as a prop must not be a `string`
17+
@Prop({ reflect: true }) val: number | any;
18+
19+
render() {
20+
this.renderCount += 1;
21+
return (
22+
<div>
23+
<div>Child Render Count: {this.renderCount}</div>
24+
<input step={this.val}></input>
25+
</div>
26+
);
27+
}
28+
29+
componentDidUpdate() {
30+
this.renderCount += 1;
31+
}
32+
}

0 commit comments

Comments
 (0)