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

fix(renderer): prevent infinite loops for NaN #3254

Merged
merged 2 commits into from
Feb 28, 2022
Merged

fix(renderer): prevent infinite loops for NaN #3254

merged 2 commits into from
Feb 28, 2022

Conversation

rwaskiewicz
Copy link
Member

@rwaskiewicz rwaskiewicz commented Feb 24, 2022 •
edited
Loading

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Unit tests (npm test) were run locally and passed
  • E2E Tests (npm run test.karma.prod) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

GitHub Issue Number: N/A

What is the new behavior?

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)

Does this introduce a breaking change?

  • Yes
  • No

Testing

The karma tests added to this PR should suffice as tests (as they emulate the original reproduction in #2828). To run them:

  1. Checkout this branch and build: npm ci && npm run build
  2. cd test/karma && npm run build.app to build the tests
  3. npm start within the karma directory to spin up the local webserver
  4. Navigate to www, then select any of the tests we added here

Other information

I ran into some issues mocking out (specifically spying on) certain methods to add unit tests here. I need to do a little research, as I'm pretty sure it's related to how we use tsconfig paths in our source and that not mapping back correctly to the test transpilation steps. For now, I think the new Karma tests should suffice. Added this research to the backlog as STENCIL-385

rwaskiewicz added a commit that referenced this pull request Feb 24, 2022
@rwaskiewicz
test(prop-parsing): add numeric prop parsing tests
af92c6f 
this commit adds tests to `src/runtime/parse-property-value.ts`.
specifically it adds tests to the case where we parse numbers using the
`parsePropertyValue` function.

these tests were originally a part of
#3254, but were removed from
the pull request. the reason for which is the fix implementation in 3254
ultimately went on a different layer of abstraction then
`parse-property-value`. these tests were originally written with a
different fix in mind; rather than scraping them, they've been updated
to fit the current implementation of the function under test
@rwaskiewicz rwaskiewicz mentioned this pull request Feb 24, 2022
Merged
14 tasks
rwaskiewicz added a commit that referenced this pull request Feb 24, 2022
@rwaskiewicz
test(prop-parsing): add numeric prop parsing tests
1146eea 
this commit adds tests to `src/runtime/parse-property-value.ts`.
specifically it adds tests to the case where we parse numbers using the
`parsePropertyValue` function.

these tests were originally a part of
#3254, but were removed from
the pull request. the reason for which is the fix implementation in 3254
ultimately went on a different layer of abstraction then
`parse-property-value`. these tests were originally written with a
different fix in mind; rather than scraping them, they've been updated
to fit the current implementation of the function under test

the intent with this pull request is not to cover every corner case of
`parseFloat`, which is the underlying implementation for this section of
the code. rather it is to document how the function under test will
respond to inputs across different types and of different numeric values
@rwaskiewicz
fix(renderer): prevent infinite loops for NaN
a03a847 
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)
@rwaskiewicz rwaskiewicz marked this pull request as ready for review February 24, 2022 23:15
@rwaskiewicz rwaskiewicz requested a review from a team February 24, 2022 23:15
@rwaskiewicz rwaskiewicz merged commit e2d4e16 into main Feb 28, 2022
@rwaskiewicz rwaskiewicz deleted the rwaskiewicz/fix-nan-inf-loop branch February 28, 2022 17:41
rwaskiewicz added a commit that referenced this pull request Feb 28, 2022
@rwaskiewicz
test(prop-parsing): add numeric prop parsing tests (#3255)
eee1c91 
this commit adds tests to `src/runtime/parse-property-value.ts`.
specifically it adds tests to the case where we parse numbers using the
`parsePropertyValue` function.

these tests were originally a part of
#3254, but were removed from
the pull request. the reason for which is the fix implementation in 3254
ultimately went on a different layer of abstraction then
`parse-property-value`. these tests were originally written with a
different fix in mind; rather than scraping them, they've been updated
to fit the current implementation of the function under test

the intent with this pull request is not to cover every corner case of
`parseFloat`, which is the underlying implementation for this section of
the code. rather it is to document how the function under test will
respond to inputs across different types and of different numeric values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ltm ltm ltm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rwaskiewicz @ltm