Issue/#19545 #19584

pfongkye · 2020-08-11T01:22:21Z

Summary

Fixes #19545
The data.name in a given payload should not be a function since window.postMessage would throw an error.

If I'm on the right path, should we modify formatDataForPreview to take into consideration the fact that data.name can be an anonymous function?

Test Plan

Tests to be added

facebook-github-bot · 2020-08-11T01:22:32Z

Hi @pfongkye!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, 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.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

codesandbox-ci · 2020-08-11T01:24:40Z

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 93543e0:

Sandbox	Source
React	Configuration
distracted-wozniak-q8kdb	Issue #19545

sizebot · 2020-08-11T01:27:19Z

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 93543e0

sizebot · 2020-08-11T01:27:47Z

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 93543e0

facebook-github-bot · 2020-08-11T01:45:47Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot · 2020-08-11T02:43:43Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot · 2020-08-11T04:37:26Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

omarsy · 2020-08-11T06:15:02Z

packages/react-devtools-shared/src/hydration.js

+          typeof data.name === 'function' || !data.name
+            ? 'function'
+            : data.name,


The javascript Proxy wraps the name attribute. I think instead of using data.name you can replace it by data.prototype.constructor.name. It is the same thing but the difference is the constructor.name is not wrapped by the proxy.

The javascript Proxy wraps the name attribute.

Oh that's very interesting! Nice find.

I think instead of using data.name you can replace it by data.prototype.constructor.name. It is the same thing but the difference is the constructor.name is not wrapped by the proxy.

I'm wary of using data.prototype.constructor.name because not all objects have a prototype (e.g. Object.create(null)

bvaughn · 2020-08-11T16:29:11Z

If I'm on the right path, should we modify formatDataForPreview to take into consideration the fact that data.name can be an anonymous function?

Yes. Sounds worth doing.

pfongkye · 2020-08-11T21:26:51Z

@bvaughn I had some tests failing before the modifications. I fixed some. There are two remaining. I wanted to check with you if these are the new expected behaviors before fixing them:
snapshot failing
expecting 3 but received 1

thehanimo · 2020-08-12T11:54:28Z

packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js

@@ -403,13 +403,13 @@ describe('InspectedElementContext', () => {

    expect(targetRenderCount).toBe(1);
    expect(errorSpy).toHaveBeenCalledTimes(1);
-    expect(errorSpy).toHaveBeenCalledWith('error');
+    expect(errorSpy).toHaveBeenCalledWith('Warning: error');


Umm.. does Warning: error look okay? The only tests failing in CircleCI belong to this specific commit (args prefixed with "Warning: ").

bvaughn · 2020-08-12T14:40:35Z

@pfongkye I'm not sure I understand what you're asking about those two tests. How do they fail? Hard for me to say if the failure would be expected or not without having more context. The second one in particular though (expected 3 but got 1) does not seem expected, I think.

bvaughn · 2020-08-12T14:41:00Z

packages/react-devtools-shared/src/__tests__/bridge-test.js

@@ -43,7 +43,7 @@ describe('Bridge', () => {
    jest.runAllTimers();
    expect(wall.send).not.toHaveBeenCalled();
    expect(console.warn).toHaveBeenCalledWith(
-      'Cannot send message "should not send" through a Bridge that has been shutdown.',
+      'Warning: Cannot send message "should not send" through a Bridge that has been shutdown.',


Why are you adding "Warning:" prefixes to these tests?

…acebook#19545

pfongkye · 2020-08-12T15:27:17Z

@pfongkye I'm not sure I understand what you're asking about those two tests. How do they fail? Hard for me to say if the failure would be expected or not without having more context. The second one in particular though (expected 3 but got 1) does not seem expected, I think.

Sorry. So I don't know why but I was having those two tests failing on my computer (OS is Windows 10 Professional):

But after running build-for-devtools, I'm still having the following test failing:

I'm going to revert commit pfongkye@6b36d98#diff-ab5ee5655b2aac3260e1f836546a13c9 and hopefully find out why the tests are failing.

This reverts commit 6b36d98.

bvaughn · 2020-08-12T15:30:18Z

FWIW, I checked out your branch and reverted that commit locally– and when I ran DevTools tests, they all passed.

bvaughn

This looks like a nice improvement to me. Let's see if CI is happy 😄

Any other changes you looking to include? (I see the PR is still in "Draft" status)

pfongkye · 2020-08-12T15:46:50Z

Any other changes you looking to include? (I see the PR is still in "Draft" status)

Nope. It was because of the tests :)

bvaughn

Looks good to me, thank you!

pfongkye · 2020-08-12T17:11:59Z

Thanks a lot!
Guess I'll have to run the tests in docker (or maybe on WSL2) next time 😅

Pascal Fong Kye added 2 commits August 11, 2020 03:09

data.name should not be a function

c96e667

fix typo

c5cab33

facebook-github-bot added the CLA Signed label Aug 11, 2020

omarsy reviewed Aug 11, 2020

View reviewed changes

fear018 approved these changes Aug 11, 2020

View reviewed changes

bvaughn self-assigned this Aug 11, 2020

Pascal Fong Kye added 2 commits August 11, 2020 22:17

fix: assuming new expected behaviors

6b36d98

format anonymous function name from proxy

6c9945e

thehanimo reviewed Aug 12, 2020

View reviewed changes

bvaughn reviewed Aug 12, 2020

View reviewed changes

Merge branch 'master' of https://github.com/facebook/react into issue/f…

c5dfddf

…acebook#19545

Revert "fix: assuming new expected behaviors"

93543e0

This reverts commit 6b36d98.

bvaughn reviewed Aug 12, 2020

View reviewed changes

pfongkye marked this pull request as ready for review August 12, 2020 15:47

bvaughn approved these changes Aug 12, 2020

View reviewed changes

bvaughn merged commit b6e1d08 into facebook:master Aug 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/#19545 #19584

Issue/#19545 #19584

pfongkye commented Aug 11, 2020 •

edited

Loading

facebook-github-bot commented Aug 11, 2020

codesandbox-ci bot commented Aug 11, 2020 •

edited

Loading

sizebot commented Aug 11, 2020 •

edited

Loading

sizebot commented Aug 11, 2020 •

edited

Loading

facebook-github-bot commented Aug 11, 2020

facebook-github-bot commented Aug 11, 2020

facebook-github-bot commented Aug 11, 2020

omarsy Aug 11, 2020

bvaughn Aug 11, 2020

bvaughn commented Aug 11, 2020

pfongkye commented Aug 11, 2020

thehanimo Aug 12, 2020

bvaughn commented Aug 12, 2020

bvaughn Aug 12, 2020

pfongkye commented Aug 12, 2020

bvaughn commented Aug 12, 2020

bvaughn left a comment

pfongkye commented Aug 12, 2020 •

edited

Loading

bvaughn left a comment

pfongkye commented Aug 12, 2020

Issue/#19545 #19584

Issue/#19545 #19584

Conversation

pfongkye commented Aug 11, 2020 • edited Loading

Summary

Test Plan

facebook-github-bot commented Aug 11, 2020

codesandbox-ci bot commented Aug 11, 2020 • edited Loading

sizebot commented Aug 11, 2020 • edited Loading

Size changes (experimental)

sizebot commented Aug 11, 2020 • edited Loading

Size changes (stable)

facebook-github-bot commented Aug 11, 2020

facebook-github-bot commented Aug 11, 2020

facebook-github-bot commented Aug 11, 2020

omarsy Aug 11, 2020

Choose a reason for hiding this comment

bvaughn Aug 11, 2020

Choose a reason for hiding this comment

bvaughn commented Aug 11, 2020

pfongkye commented Aug 11, 2020

thehanimo Aug 12, 2020

Choose a reason for hiding this comment

bvaughn commented Aug 12, 2020

bvaughn Aug 12, 2020

Choose a reason for hiding this comment

pfongkye commented Aug 12, 2020

bvaughn commented Aug 12, 2020

bvaughn left a comment

Choose a reason for hiding this comment

pfongkye commented Aug 12, 2020 • edited Loading

bvaughn left a comment

Choose a reason for hiding this comment

pfongkye commented Aug 12, 2020

pfongkye commented Aug 11, 2020 •

edited

Loading

codesandbox-ci bot commented Aug 11, 2020 •

edited

Loading

sizebot commented Aug 11, 2020 •

edited

Loading

sizebot commented Aug 11, 2020 •

edited

Loading

pfongkye commented Aug 12, 2020 •

edited

Loading