The Wayback Machine - https://web.archive.org/web/20201023152025/https://github.com/facebook/react/pull/19867
Skip to content

/ react

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

Jump to bottom

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 #19867

Open
wants to merge 2 commits into
base: master
from
Open

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 #19867

wants to merge 2 commits into from

Conversation

@omarsy
Copy link
Contributor

@omarsy omarsy commented Sep 19, 2020 •
edited

Fixes #19848

Summary

image

Test Plan
@omarsy
support symbol when deps argument passed to useEffect changed
Loading status checks…
437ddb3
@codesandbox
Copy link

@codesandbox codesandbox bot commented Sep 19, 2020 •
edited

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5ef6705:

Sandbox Source
React Configuration
hungry-frog-dg1q9 Issue #19848
@sizebot
Copy link

@sizebot sizebot commented Sep 19, 2020 •
edited

Details of bundled changes.

Comparing: 8b2d378...5ef6705

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 908.28 KB 908.84 KB 206.4 KB 206.54 KB NODE_DEV
ReactDOMForked-prod.js 0.0% -0.0% 375.79 KB 375.79 KB 69.69 KB 69.69 KB FB_WWW_PROD
react-dom-server.node.development.js +0.4% +0.4% 138.57 KB 139.13 KB 36.59 KB 36.74 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 122.28 KB 122.28 KB 39.34 KB 39.34 KB NODE_PROD
react-dom-server.browser.development.js +0.4% +0.4% 144.73 KB 145.31 KB 36.77 KB 36.91 KB UMD_DEV
react-dom-server.node.production.min.js 0.0% 0.0% 20.66 KB 20.66 KB 7.65 KB 7.65 KB NODE_PROD
react-dom-test-utils.production.min.js 0.0% 0.0% 13.66 KB 13.66 KB 5.31 KB 5.32 KB UMD_PROD
ReactDOMTesting-dev.js +0.1% +0.1% 912.12 KB 912.68 KB 205.29 KB 205.44 KB FB_WWW_DEV
react-dom-test-utils.development.js 0.0% 0.0% 65.06 KB 65.06 KB 18.63 KB 18.63 KB NODE_DEV
react-dom-unstable-fizz.node.development.js 0.0% -0.1% 5.52 KB 5.52 KB 1.84 KB 1.84 KB NODE_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.2% 1.17 KB 1.17 KB 666 B 667 B NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.22 KB 1.22 KB 713 B 712 B UMD_PROD
react-dom.development.js +0.1% +0.1% 954.43 KB 955.02 KB 209.01 KB 209.16 KB UMD_DEV
react-dom.production.min.js 0.0% 0.0% 122.11 KB 122.11 KB 40.09 KB 40.09 KB UMD_PROD
ReactDOMForked-dev.js +0.1% +0.1% 962.75 KB 963.32 KB 214.25 KB 214.4 KB FB_WWW_DEV
ReactDOM-dev.js +0.1% +0.1% 961.26 KB 961.82 KB 214.66 KB 214.8 KB FB_WWW_DEV
react-dom-server.browser.development.js +0.4% +0.4% 137.3 KB 137.86 KB 36.34 KB 36.49 KB NODE_DEV
ReactDOMServer-dev.js +0.4% +0.4% 141.5 KB 142.06 KB 36.28 KB 36.42 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% 0.0% 46.44 KB 46.44 KB 10.83 KB 10.83 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% 0.0% 70.22 KB 70.22 KB 19.12 KB 19.12 KB UMD_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.1% +0.1% 650.27 KB 650.83 KB 138.67 KB 138.81 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% 🔺+0.1% 2.8 KB 2.8 KB 1.16 KB 1.16 KB NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-prod.js 0.0% -0.0% 233.09 KB 233.09 KB 41.38 KB 41.37 KB FB_WWW_PROD
react-art.development.js +0.1% +0.1% 691.64 KB 692.22 KB 146.6 KB 146.75 KB UMD_DEV
react-art.development.js +0.1% +0.1% 592.78 KB 593.34 KB 128.72 KB 128.86 KB NODE_DEV
ReactART-dev.js +0.1% +0.1% 619.36 KB 619.92 KB 131.58 KB 131.74 KB FB_WWW_DEV

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.1% +0.1% 605.92 KB 606.51 KB 127.64 KB 127.79 KB UMD_DEV
react-test-renderer.production.min.js 0.0% -0.0% 76.42 KB 76.42 KB 24.06 KB 24.06 KB UMD_PROD
react-test-renderer.development.js +0.1% +0.1% 577.21 KB 577.77 KB 126.17 KB 126.31 KB NODE_DEV
ReactTestRenderer-dev.js +0.1% +0.1% 592.34 KB 592.9 KB 127.31 KB 127.45 KB FB_WWW_DEV
ReactTestRenderer-dev.js +0.1% +0.1% 587 KB 587.56 KB 127.08 KB 127.21 KB RN_FB_DEV
ReactTestRenderer-prod.js 0.0% 0.0% 229.31 KB 229.31 KB 41.92 KB 41.92 KB RN_FB_PROD
ReactTestRenderer-profiling.js 0.0% 0.0% 240.66 KB 240.66 KB 44.06 KB 44.06 KB RN_FB_PROFILING

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.1% +0.1% 677.47 KB 678.03 KB 146.97 KB 147.1 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 0.0% -0.0% 268.02 KB 268.02 KB 47.91 KB 47.91 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js 0.0% -0.0% 279.5 KB 279.5 KB 50.11 KB 50.11 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.1% +0.1% 658.11 KB 658.67 KB 142.33 KB 142.46 KB RN_OSS_DEV

ReactDOM: size: 0.0%, gzip: 0.0%

Size changes (experimental)

Generated by 🚫 dangerJS against 5ef6705

@sizebot
Copy link

@sizebot sizebot commented Sep 19, 2020 •
edited

Details of bundled changes.

Comparing: 8b2d378...5ef6705

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 872.83 KB 873.39 KB 199.9 KB 200.04 KB NODE_DEV
ReactDOMForked-prod.js 0.0% -0.0% 387.12 KB 387.12 KB 71.45 KB 71.45 KB FB_WWW_PROD
react-dom-server.node.development.js +0.4% +0.4% 137.06 KB 137.62 KB 36.37 KB 36.53 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 117.72 KB 117.72 KB 38.02 KB 38.03 KB NODE_PROD
react-dom-server.browser.development.js +0.4% +0.4% 143.14 KB 143.73 KB 36.57 KB 36.72 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 13.65 KB 13.65 KB 5.31 KB 5.31 KB UMD_PROD
ReactDOMTesting-dev.js +0.1% +0.1% 940.45 KB 941.02 KB 210.76 KB 210.9 KB FB_WWW_DEV
react-dom-test-utils.development.js 0.0% 0.0% 65.05 KB 65.05 KB 18.62 KB 18.62 KB NODE_DEV
ReactDOMTesting-prod.js 0.0% 0.0% 385.05 KB 385.05 KB 72.38 KB 72.38 KB FB_WWW_PROD
react-dom-test-utils.production.min.js 0.0% 0.0% 13.58 KB 13.58 KB 5.21 KB 5.21 KB NODE_PROD
react-dom.development.js +0.1% +0.1% 917.24 KB 917.82 KB 202.44 KB 202.58 KB UMD_DEV
ReactDOMForked-dev.js +0.1% +0.1% 988.34 KB 988.9 KB 218.96 KB 219.11 KB FB_WWW_DEV
ReactDOM-dev.js +0.1% +0.1% 986.84 KB 987.4 KB 219.32 KB 219.46 KB FB_WWW_DEV
react-dom-server.browser.development.js +0.4% +0.4% 135.79 KB 136.35 KB 36.12 KB 36.28 KB NODE_DEV
ReactDOM-profiling.js 0.0% 0.0% 399.04 KB 399.04 KB 73.53 KB 73.53 KB FB_WWW_PROFILING
ReactDOMServer-dev.js +0.4% +0.4% 145.53 KB 146.09 KB 37.3 KB 37.44 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% 0.0% 47.3 KB 47.3 KB 11.03 KB 11.04 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% 0.0% 70.21 KB 70.21 KB 19.11 KB 19.11 KB UMD_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.1% +0.1% 682.85 KB 683.41 KB 147.83 KB 147.97 KB RN_FB_DEV
ReactFabric-dev.js +0.1% +0.1% 663.5 KB 664.06 KB 143.2 KB 143.34 KB RN_FB_DEV
ReactNativeRenderer-dev.js +0.1% +0.1% 677.45 KB 678.02 KB 146.96 KB 147.1 KB RN_OSS_DEV
ReactFabric-prod.js 0.0% -0.0% 261.69 KB 261.69 KB 46.59 KB 46.59 KB RN_FB_PROD
ReactNativeRenderer-prod.js 0.0% -0.0% 268.01 KB 268.01 KB 47.9 KB 47.9 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js 0.0% -0.0% 279.48 KB 279.48 KB 50.1 KB 50.1 KB RN_OSS_PROFILING
ReactNativeRenderer-prod.js 0.0% -0.0% 267.97 KB 267.97 KB 47.88 KB 47.88 KB RN_FB_PROD
ReactNativeRenderer-profiling.js 0.0% -0.0% 279.44 KB 279.44 KB 50.08 KB 50.08 KB RN_FB_PROFILING
ReactFabric-dev.js +0.1% +0.1% 658.1 KB 658.66 KB 142.32 KB 142.45 KB RN_OSS_DEV
ReactFabric-prod.js 0.0% -0.0% 261.72 KB 261.72 KB 46.61 KB 46.61 KB RN_OSS_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.1% +0.1% 622.14 KB 622.7 KB 133.18 KB 133.32 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% 🔺+0.1% 2.79 KB 2.79 KB 1.15 KB 1.15 KB NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-prod.js 0.0% -0.0% 240.25 KB 240.25 KB 42.63 KB 42.63 KB FB_WWW_PROD
react-art.development.js +0.1% +0.1% 664.73 KB 665.32 KB 141.6 KB 141.75 KB UMD_DEV
react-art.development.js +0.1% +0.1% 567.14 KB 567.7 KB 123.82 KB 123.95 KB NODE_DEV
ReactART-dev.js +0.1% +0.1% 629.37 KB 629.93 KB 133.61 KB 133.76 KB FB_WWW_DEV

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.1% +0.1% 605.9 KB 606.48 KB 127.63 KB 127.77 KB UMD_DEV
react-test-renderer.development.js +0.1% +0.1% 577.18 KB 577.74 KB 126.16 KB 126.29 KB NODE_DEV
react-test-renderer.production.min.js 0.0% -0.0% 76.19 KB 76.19 KB 23.73 KB 23.73 KB NODE_PROD
ReactTestRenderer-dev.js +0.1% +0.1% 592.33 KB 592.89 KB 127.3 KB 127.44 KB FB_WWW_DEV
ReactTestRenderer-dev.js +0.1% +0.1% 586.99 KB 587.55 KB 127.07 KB 127.21 KB RN_FB_DEV
ReactTestRenderer-prod.js 0.0% 0.0% 229.3 KB 229.3 KB 41.91 KB 41.91 KB RN_FB_PROD
ReactTestRenderer-profiling.js 0.0% 0.0% 240.65 KB 240.65 KB 44.05 KB 44.05 KB RN_FB_PROFILING

ReactDOM: size: 0.0%, gzip: 0.0%

Size changes (stable)

Generated by 🚫 dangerJS against 5ef6705

@omarsy
handle the case if len equal to 0
Loading status checks…
5ef6705
@omarsy
omarsy reviewed Sep 19, 2020
View changes
packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -194,6 +194,20 @@ let hookTypesUpdateIndexDev: number = -1;
// When true, such Hooks will always be "remounted". Only used during hot reload.
let ignorePreviousDependencies: boolean = false;

function joinArray(collection: Array<mixed>, separator: string = ''): string {

This comment has been minimized.

Sign in to view
@omarsy

omarsy Sep 19, 2020
Author Contributor

Maybe we can move this function in another file ?
@leidegre
Copy link
Contributor

@leidegre leidegre commented Sep 29, 2020

Since you did make seperate function you might as well apply it recursively. Otherwise, the error will still happen if you for some reason have ''+[[Symbol()]] (nested arrays with symbols) and since this is a helper function for a very specific code path I would nest the function under the if (__DEV__) statement unless there's something in the coding guidelines against this because it more clearly communicates that this function exists to solve a very specific problem.

With that in mind, I think a comment about why it's done like this is prudent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed
4 participants
@omarsy @sizebot @leidegre @facebook-github-bot
You can’t perform that action at this time.