Skip to content

Bug: missing else statement in ReactDOMServerFormatConfig.js #22309

Not planned
@justingrant

Description

@justingrant
Contributor

I found this bug while working on an unrelated PR #22064. Looks like there's a missing else, because the value set in line 978 is always overwritten in line 980. Given that children.toString() and children[0].toString() will return the same string for one-element arrays, I assume the best (for perf and bundle size) fix would be to remove the assignment inside the if block.

if (isArray(children)) {
invariant(
children.length <= 1,
'<textarea> can only have at most one child.',
);
value = '' + children[0];
}
value = '' + children;

Because the observable behavior is identical whether or not children is a scalar or a one-element array, I'm not sure a test could be written to verify a fix.

EDIT: I updated the text above after I learned that Array.prototype.toString() acts the same as Array.prototype.join(). JavaScript teaches me something new every day!

Activity

akgupta0777

akgupta0777 commented on Sep 14, 2021

@akgupta0777
Contributor

@justingrant It is using an invariant in the if check. Invariant is an error handling guard which lets you use assertions in your code.
so invariant requires two arguments first one is the condition and second is error message to be thrown after condition gets violated.

In above case, As soon as length of children is more than 1 then it throws an error saying '<textarea> can only have at most one child.' and stops the execution right away and prevent overwriting of value.

justingrant

justingrant commented on Sep 14, 2021

@justingrant
ContributorAuthor

In above case, As soon as length of children is more than 1 then it throws an error saying '<textarea> can only have at most one child.' and stops the execution right away and prevent overwriting of value.

I was actually thinking about the 1-element-in-array case. I'd assumed that coercing an Array to a string would yield something like '[object Array]'. But just now I learned that Array overrides toString and its implementation is equivalent to .join()! After 20 years of using JS, every time I think I've learned enough, I'm reminded that there are still surprises lurking.

So there's still a bug here, but it's a much minor one than I originally thought. The actual bug is that value = '' + children[0]; is unnecessary code and can be removed. If there's one element in the array, execution will fall through to line 980 and the original value will be overwritten with the same exact string.

akgupta0777

akgupta0777 commented on Sep 14, 2021

@akgupta0777
Contributor

Seriously JavaScript sometimes screw minds really well. Even today I also got to know that concatenation of an array with string results in a string with all array elements joined separated by ",".
Well yes in that case it is unnecessary to use line 978.

I will love to contribute on this issue and fix this bug.

rickhanlonii

rickhanlonii commented on Sep 15, 2021

@rickhanlonii
Member

Was added here, and does look like a mistake. cc @sebmarkbage

added and removed
Status: UnconfirmedA potential issue that we haven't yet confirmed as a bug
on Sep 15, 2021
sebmarkbage

sebmarkbage commented on Sep 15, 2021

@sebmarkbage
Collaborator

Yea, feel free to fix it to an else. Either solution works and might deopt differently but given that this whole branch warns anyway it doesn't matter.

Hyperion101010

Hyperion101010 commented on Sep 24, 2021

@Hyperion101010

Picking this up

justingrant

justingrant commented on Sep 24, 2021

@justingrant
ContributorAuthor

15 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @sebmarkbage@justingrant@rickhanlonii@amensum@Hyperion101010

      Issue actions

        Bug: missing `else` statement in ReactDOMServerFormatConfig.js · Issue #22309 · facebook/react