-
Notifications
You must be signed in to change notification settings - Fork 789
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(renderer): prevent infinite loops for NaN (#3254)
this commit is intended to prevent infinite loops when rendering the
following specific scenario:
- the prop is of type "number"
- the prop is reflected (`@Prop({reflect: true})`
- the value received by the prop is NaN
in this commit, we add an additional safeguard to `set-value` to
validate that both the previous and new values on the prop are not NaN.
this check is required as equality checks in javascript where NaN is on
both the left and right hand side of the operator will always return
false.
this change is intentionally placed as this layer of abstraction in
order to handle cases where a value may be set on a prop from different
places in the code base. for instance, we must consider cases where:
- a reflected camelCase prop 'myProp' is reflected as 'my-prop'
- a reflected single word prop 'val' is reflected as 'val' (no
transformation)- Loading branch information
1 parent
a04aa22
commit e2d4e16
Showing
12 changed files
with
258 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| <!DOCTYPE html> | ||
| <meta charset="utf8"> | ||
| <script src="./build/testapp.esm.js" type="module"></script> | ||
| <script src="./build/testapp.js" nomodule></script> | ||
|
|
||
| <!-- The string 'NaN' will be interpreted as a number by Stencil, based on the type declaration on the prop tied to --> | ||
| <!-- the 'val-num' attribute --> | ||
| <reflect-nan-attribute-hyphen val-num='NaN'></reflect-nan-attribute-hyphen> |
20 changes: 20 additions & 0 deletions
20
test/karma/test-app/reflect-nan-attribute-hyphen/karma.spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import { setupDomTests } from '../util'; | ||
|
|
||
| describe('reflect-nan-attribute-hyphen', () => { | ||
| const { setupDom, tearDownDom } = setupDomTests(document); | ||
| let app: HTMLElement; | ||
|
|
||
| beforeEach(async () => { | ||
| app = await setupDom('/reflect-nan-attribute-hyphen/index.html'); | ||
| }); | ||
| afterEach(tearDownDom); | ||
|
|
||
| it('renders the component the correct number of times', async () => { | ||
| const cmpShadowRoot = app.querySelector('reflect-nan-attribute-hyphen')?.shadowRoot; | ||
| if (!cmpShadowRoot) { | ||
| fail(`unable to find shadow root on component 'reflect-nan-attribute-hyphen'`); | ||
| } | ||
|
|
||
| expect(cmpShadowRoot.textContent).toEqual('reflect-nan-attribute-hyphen Render Count: 1'); | ||
| }); | ||
| }); |
26 changes: 26 additions & 0 deletions
26
test/karma/test-app/reflect-nan-attribute-hyphen/reflect-nan-attribute-hyphen.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { Component, Prop, h } from '@stencil/core'; | ||
|
|
||
| @Component({ | ||
| tag: 'reflect-nan-attribute-hyphen', | ||
| // 'shadow' is not needed here, but does make testing easier by using the shadow root to help encapsulate textContent | ||
| shadow: true, | ||
| }) | ||
| export class ReflectNanAttributeHyphen { | ||
| // for this test, it's necessary that 'reflect' is true, the class member is camel-cased, and is of type 'number' | ||
| @Prop({ reflect: true }) valNum: number; | ||
|
|
||
| // counter to proxy the number of times a render has occurred, since we don't have access to those dev tools during | ||
| // karma tests | ||
| renderCount = 0; | ||
|
|
||
| render() { | ||
| this.renderCount += 1; | ||
| return <div>reflect-nan-attribute-hyphen Render Count: {this.renderCount}</div>; | ||
| } | ||
|
|
||
| componentDidUpdate() { | ||
| // we don't expect the component to update, this will increment the render count in case it does (and should fail | ||
| // the test) | ||
| this.renderCount += 1; | ||
| } | ||
| } |
26 changes: 26 additions & 0 deletions
26
test/karma/test-app/reflect-nan-attribute-with-child/child-reflect-nan-attribute.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { Component, h, Prop } from '@stencil/core'; | ||
|
|
||
| @Component({ | ||
| tag: 'child-reflect-nan-attribute', | ||
| // 'shadow' is not needed here, but does make testing easier by using the shadow root to help encapsulate textContent | ||
| shadow: true, | ||
| }) | ||
| export class ChildReflectNanAttribute { | ||
| // for this test, it's necessary that 'reflect' is true, the class member is not camel-cased, and is of type 'number' | ||
| @Prop({ reflect: true }) val: number; | ||
|
|
||
| // counter to proxy the number of times a render has occurred, since we don't have access to those dev tools during | ||
| // karma tests | ||
| renderCount = 0; | ||
|
|
||
| render() { | ||
| this.renderCount += 1; | ||
| return <div>child-reflect-nan-attribute Render Count: {this.renderCount}</div>; | ||
| } | ||
|
|
||
| componentDidUpdate() { | ||
| // we don't expect the component to update, this will increment the render count in case it does (and should fail | ||
| // the test) | ||
| this.renderCount += 1; | ||
| } | ||
| } |
6 changes: 6 additions & 0 deletions
6
test/karma/test-app/reflect-nan-attribute-with-child/index.html
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| <!DOCTYPE html> | ||
| <meta charset="utf8"> | ||
| <script src="./build/testapp.esm.js" type="module"></script> | ||
| <script src="./build/testapp.js" nomodule></script> | ||
|
|
||
| <parent-reflect-nan-attribute></parent-reflect-nan-attribute> |
26 changes: 26 additions & 0 deletions
26
test/karma/test-app/reflect-nan-attribute-with-child/karma.spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { setupDomTests } from '../util'; | ||
|
|
||
| describe('reflect-nan-attribute-with-child', () => { | ||
| const { setupDom, tearDownDom } = setupDomTests(document); | ||
| let app: HTMLElement; | ||
|
|
||
| beforeEach(async () => { | ||
| app = await setupDom('/reflect-nan-attribute-with-child/index.html'); | ||
| }); | ||
| afterEach(tearDownDom); | ||
|
|
||
| it('renders the parent and child the correct number of times', async () => { | ||
| const parentShadowRoot = app.querySelector('parent-reflect-nan-attribute')?.shadowRoot; | ||
| if (!parentShadowRoot) { | ||
| fail(`unable to find shadow root on component 'parent-reflect-nan-attribute'`); | ||
| } | ||
|
|
||
| const childShadowRoot = parentShadowRoot.querySelector('child-reflect-nan-attribute')?.shadowRoot; | ||
| if (!childShadowRoot) { | ||
| fail(`unable to find shadow root on component 'child-with-reflection'`); | ||
| } | ||
|
|
||
| expect(parentShadowRoot.textContent).toEqual('parent-reflect-nan-attribute Render Count: 1'); | ||
| expect(childShadowRoot.textContent).toEqual('child-reflect-nan-attribute Render Count: 1'); | ||
| }); | ||
| }); |
30 changes: 30 additions & 0 deletions
30
test/karma/test-app/reflect-nan-attribute-with-child/parent-reflect-nan-attribute.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import { Component, h } from '@stencil/core'; | ||
|
|
||
| @Component({ | ||
| tag: 'parent-reflect-nan-attribute', | ||
| // 'shadow' is not needed here, but does make testing easier by using the shadow root to help encapsulate textContent | ||
| shadow: true, | ||
| }) | ||
| export class ParentReflectNanAttribute { | ||
| // counter to proxy the number of times a render has occurred, since we don't have access to those dev tools during | ||
| // karma tests | ||
| renderCount = 0; | ||
|
|
||
| render() { | ||
| this.renderCount += 1; | ||
| return ( | ||
| <div> | ||
| <div>parent-reflect-nan-attribute Render Count: {this.renderCount}</div> | ||
| {/* | ||
| // @ts-ignore */} | ||
| <child-reflect-nan-attribute val={'I am not a number!!'}></child-reflect-nan-attribute> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| componentDidUpdate() { | ||
| // we don't expect the component to update, this will increment the render count in case it does (and should fail | ||
| // the test) | ||
| this.renderCount += 1; | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| <!DOCTYPE html> | ||
| <meta charset="utf8"> | ||
| <script src="./build/testapp.esm.js" type="module"></script> | ||
| <script src="./build/testapp.js" nomodule></script> | ||
|
|
||
| <!-- The string 'NaN' will be interpreted as a number by Stencil, based on the type declaration on the prop tied to --> | ||
| <!-- the 'val' attribute --> | ||
| <reflect-nan-attribute val='NaN'></reflect-nan-attribute> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import { setupDomTests } from '../util'; | ||
|
|
||
| describe('reflect-nan-attribute', () => { | ||
| const { setupDom, tearDownDom } = setupDomTests(document); | ||
| let app: HTMLElement; | ||
|
|
||
| beforeEach(async () => { | ||
| app = await setupDom('/reflect-nan-attribute/index.html'); | ||
| }); | ||
| afterEach(tearDownDom); | ||
|
|
||
| it('renders the component the correct number of times', async () => { | ||
| const cmpShadowRoot = app.querySelector('reflect-nan-attribute')?.shadowRoot; | ||
| if (!cmpShadowRoot) { | ||
| fail(`unable to find shadow root on component 'reflect-nan-attribute'`); | ||
| } | ||
|
|
||
| expect(cmpShadowRoot.textContent).toEqual('reflect-nan-attribute Render Count: 1'); | ||
| }); | ||
| }); |
26 changes: 26 additions & 0 deletions
26
test/karma/test-app/reflect-nan-attribute/reflect-nan-attribute.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { Component, Prop, h } from '@stencil/core'; | ||
|
|
||
| @Component({ | ||
| tag: 'reflect-nan-attribute', | ||
| // 'shadow' is not needed here, but does make testing easier by using the shadow root to help encapsulate textContent | ||
| shadow: true, | ||
| }) | ||
| export class ReflectNanAttribute { | ||
| // for this test, it's necessary that 'reflect' is true, the class member is not camel-cased, and is of type 'number' | ||
| @Prop({ reflect: true }) val: number; | ||
|
|
||
| // counter to proxy the number of times a render has occurred, since we don't have access to those dev tools during | ||
| // karma tests | ||
| renderCount = 0; | ||
|
|
||
| render() { | ||
| this.renderCount += 1; | ||
| return <div>reflect-nan-attribute Render Count: {this.renderCount}</div>; | ||
| } | ||
|
|
||
| componentDidUpdate() { | ||
| // we don't expect the component to update, this will increment the render count in case it does (and should fail | ||
| // the test) | ||
| this.renderCount += 1; | ||
| } | ||
| } |