-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
fix(custom-elements): cast numbers with Number rules #4393
Conversation
Size report
|
did we have two |
It should definitely be renamed to something else |
@@ -0,0 +1,36 @@ | |||
import { parseNumber } from '../src/apiCustomElement' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test file customElement.spec.ts
already exists 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cmd + P betrayed me! 😆
Internally Vue was using `parseFloat` for this, and `parseFloat('1abc') === 1` rather than `NaN` like it is for non-numeric strings The fix was lifted from vuejs/core#4393, so I'll double check whether that's been merged by the time the rest of this is ready
While this is a good improvement, I took another look at #4370 and feels there's a bit more to the original issue. Specifically for a prop declared as |
Yeah, I thought about it too. I have two questions then:
|
What would it take to get this fix included in a near term version push. It is holding up some of our pushes. |
Getting feedback on the two questions above from people using this would help move this forward |
On Vue 2, all <av-component data-is-open="true" data-id="xpd154" data-amount="12.18"></av-component>
In this case type assertion based on the prop type within |
Fix #4370
By using
Number()
, we handle Infinity, NaN, and all other number syntaxes. I think this makes the most sense for the custom elements since it's also "the platform" and Infinity and NaN can be conveniently casted to a string too (String(Number('Infinity')) === 'Infinity')
).We could also take a look at the type of the prop being a Number and only apply the cast in that case. Let me know if that makes more sense.