React: Bug: Handling of symbols when used as deps incorrectly to create error message results in an unrelated TypeError: Cannot convert a Symbol value to a string

[leidegre]

React version: 16.3.1

Steps To Reproduce

1. Click the button in this Code Sandbox https://codesandbox.io/s/blissful-sun-e0lle?file=/src/App.js

The current behavior

The wrong error is generated

The expected behavior

An error should still be generated but the error message should be correct. The problem is that if you do [Symbol('...')].join(',') JavaScript will freak out which is what happens if you put symbols incorrectly in the deps to hooks.

[eps1lon]

Basically comes down to https://github.com/facebook/react/blob/ddd1faa1972b614dfbfae205f2aa4a6c0b39a759/packages/react-dom/src/server/ReactPartialRendererHooks.js#L113

(and all implementations of areHookInputsEqual) assuming that each value in the array can be (implicitly) converted to a string. Symbols can't though. nextDeps.map(dep => String(dep)).join(', ') seems like the safer option.

[omarsy]

I can have a look if you want @eps1lon ?

[eps1lon]

I can have a look if you want @eps1lon ?

You can but I don't decide what gets merged or should get merged. A core member might still decide that this is e.g. too expensive. Just managing expectations here.

[omarsy]

Ok @eps1lon I'm going to benchmark my changes before I push a PR

[gaearon]

This is DEV-only so it's fine to do a bit more work.

[leidegre]

Ok @eps1lon I'm going to benchmark my changes before I push a PR

The more efficient way to do is by doing the string concat via traditional for loop but it's completely unnecessary to think about performance here. This code will only run if you use deps incorrectly which isn't a hot path for anyone (this much I think we can be certain about). And as Dan points out, it's DEV-only. It has no impact on React in production.