Skip to content
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

Don't use Spread in DevTools Injection #18277

Merged
merged 1 commit into from
Mar 11, 2020

Conversation

sebmarkbage
Copy link
Collaborator

Related to this https://github.com/facebook/react/pull/18233/files#r391135037

We could encourage new arbitrary things to be added to this object through spread but that shares a namespace with our own things which risks us colliding and causing weird behavior. The Flow type defines that this is exact so to enforce that it is indeed exact, we can instead enumerate the values.

We really should be doing that everywhere in our internals, like we do with fiber cloning for example, and ideally avoid Object.assign as much as possible even in user facing APIs.

In DEV behavior I'm not as strict but this ships in PROD.

@facebook-github-bot facebook-github-bot added React Core Team Opened by a member of the React Core Team CLA Signed labels Mar 11, 2020
bundleType: devToolsConfig.bundleType,
version: devToolsConfig.version,
rendererPackageName: devToolsConfig.rendererPackageName,
findFiberByHostInstance: devToolsConfig.findFiberByHostInstance,
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This last line would be replaced with rendererConfig if we do https://github.com/facebook/react/pull/18233/files#r391135037

@codesandbox-ci
Copy link

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 ee144e1:

Sandbox Source
runtime-hill-n0jsw Configuration

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Mar 11, 2020

I believe this change should let us compile out the intermediate steps here but there's still something about how this is wired up that confuses the compiler until leave an unnecessary closure in place. I think it's because our use of closures in findHostInstanceByFiber.

@sizebot
Copy link

sizebot commented Mar 11, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against ee144e1

@sizebot
Copy link

sizebot commented Mar 11, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against ee144e1

@sebmarkbage sebmarkbage merged commit b5c6dd2 into facebook:master Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants