[lexical-list] Bug Fix: handle non-integer numbers in setIndent #6522
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hi @jrfitzsimmons! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
size-limit report 📦
|
facebook-github-bot
added
the
CLA Signed
jrfitzsimmons
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz,
ivailop7,
Sahejkm and
potatowagon
as code owners
| typeof indent === 'number' && indent > -1, | ||
| 'Invalid indent value.', | ||
| ); | ||
| invariant(typeof indent === 'number', 'Invalid indent value.'); |
There was a problem hiding this comment.
I wonder if we need to typecheck here when the type is already guaranteed to be a number by TS. While it is true that types are optional for users I feel like if I were to typecheck everywhere it would add a lot of noise
There was a problem hiding this comment.
@acywatson had a similar comment on the PR that added this check (#4120)
There was a problem hiding this comment.
The Math.floor is going to guarantee it's a number but it could be NaN so we might still have a bad input bug?
There was a problem hiding this comment.
That would still fail the indent >= 0 check. Or are you saying remove the invariant above given the one below will catch non-numbers. My vote would be leave it in as it's more explicit as to what's going on. I.e. not relying on Math.floor to produce NaN and then comparing that to 0. Happy to update if you prefer the latter though.
There was a problem hiding this comment.
Good point, I was just looking at it in isolation and not considering both invariants. I think it's fine as-is.
There was a problem hiding this comment.
Cool. Is there anything I need to do to fix the extended tests failures? Can't see why this change would cause them to fail...
Description
Handles non-integer numbers in
ListItemNode.setIndentto prevent infinite loop.Closes #6521
Test plan
Confirmed working in the preview playground, as well as the unit test.