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

Replace ReactShallowRenderer with a dependency #18144

Merged
merged 1 commit into from
Feb 27, 2020

Conversation

NMinhNguyen
Copy link
Contributor

@NMinhNguyen NMinhNguyen commented Feb 27, 2020

Closes #17321.

Summary

I've extracted the shallow renderer out into its own package - http://npm.im/react-shallow-renderer https://github.com/NMinhNguyen/react-shallow-renderer

As discussed with @gaearon on Twitter, I didn't bother with DEV/PROD builds - the UMD build is essentially a DEV build, and CJS and ES just have process.env.NODE_ENV checks inline.

Test Plan

CI

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 27, 2020

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

Sandbox Source
jovial-euler-ie6ce Configuration

@sizebot
Copy link

sizebot commented Feb 27, 2020

Details of bundled changes.

Comparing: f9c0a45...f3d4d56

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactTestRenderer-dev.js 0.0% -0.0% 551.32 KB 551.32 KB 116.1 KB 116.1 KB FB_WWW_DEV
react-test-renderer-shallow.development.js +1.0% -3.2% 39.17 KB 39.55 KB 9.81 KB 9.49 KB UMD_DEV
react-test-renderer-shallow.production.min.js 🔺+10.9% 🔺+9.6% 11.63 KB 12.9 KB 3.59 KB 3.93 KB UMD_PROD
react-test-renderer-shallow.development.js -98.5% -96.1% 32.03 KB 491 B 8.36 KB 331 B NODE_DEV
react-test-renderer-shallow.production.min.js -97.0% -92.9% 11.76 KB 358 B 3.68 KB 266 B NODE_PROD
ReactShallowRenderer-dev.js -98.8% -96.5% 34.25 KB 438 B 8.4 KB 297 B FB_WWW_DEV
react-test-renderer.development.js 0.0% 0.0% 524.45 KB 524.45 KB 113 KB 113 KB NODE_DEV

Size changes (experimental)

Generated by 🚫 dangerJS against f3d4d56

@sizebot
Copy link

sizebot commented Feb 27, 2020

Details of bundled changes.

Comparing: f9c0a45...f3d4d56

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer-shallow.production.min.js -97.1% -93.1% 11.74 KB 345 B 3.67 KB 258 B NODE_PROD
ReactTestRenderer-dev.js 0.0% -0.0% 551.31 KB 551.31 KB 116.1 KB 116.1 KB FB_WWW_DEV
react-test-renderer-shallow.development.js +1.0% -3.2% 39.16 KB 39.53 KB 9.8 KB 9.49 KB UMD_DEV
react-test-renderer-shallow.production.min.js 🔺+10.9% 🔺+9.7% 11.62 KB 12.88 KB 3.58 KB 3.93 KB UMD_PROD
react-test-renderer-shallow.development.js -98.5% -96.2% 32.01 KB 478 B 8.35 KB 323 B NODE_DEV
ReactShallowRenderer-dev.js -98.8% -96.5% 34.25 KB 438 B 8.4 KB 297 B FB_WWW_DEV
react-test-renderer.development.js 0.0% 0.0% 524.42 KB 524.42 KB 112.99 KB 112.99 KB NODE_DEV

Size changes (stable)

Generated by 🚫 dangerJS against f3d4d56

@NMinhNguyen
Copy link
Contributor Author

When comparing the bundles, I noticed that getComponentName has changed since I first extracted the code about a month ago due to #18086. I'm wondering if there's a better way to keep the two in sync 😕

@gaearon
Copy link
Collaborator

gaearon commented Feb 27, 2020

To be fair, adding support for new component types (if even desired) will be a more likely blocker than non-ideal getComponentName names. I don't even think getComponentName is that useful when the stack is almost flat.

@NMinhNguyen
Copy link
Contributor Author

For now, I can sync my fork with the new Block type and cut another release. 16.12.1? 16.13.0?

@@ -22,6 +22,7 @@
"object-assign": "^4.1.1",
"prop-types": "^15.6.2",
"react-is": "^16.8.6",
"react-shallow-renderer": "^16.12.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit unfortunate we didn't start with 1.0.0 or similar. So that you could break it more often if necessary. Although I guess nothing prevents you from jumping to 17 later without waiting for us.

@gaearon
Copy link
Collaborator

gaearon commented Feb 27, 2020

To avoid the versioning awkwardness, we can make it 20.0.0 and pretend this never happened.

@gaearon
Copy link
Collaborator

gaearon commented Feb 27, 2020

Blocks are an experiment and definitely don't make sense in the context of shallow rendering. So feel free to ignore them.

@gaearon
Copy link
Collaborator

gaearon commented Feb 27, 2020

I think this looks good. I’ll check tomorrow.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

I've tested this in a fresh CRA project and also at FB and couldn't find any issues. I think we can get this in. Thank you! Let's be careful with future releases since we've set this as a caret (^).

@gaearon gaearon merged commit 293878e into facebook:master Feb 27, 2020
@NMinhNguyen NMinhNguyen deleted the react-shallow-renderer branch February 27, 2020 18:12
@NMinhNguyen
Copy link
Contributor Author

I've tested this in a fresh CRA project and also at FB and couldn't find any issues.

Just wondering what kinds of tests you ran exactly, so I could try and run them myself for future releases of the package (maybe even have them run on CI).

@gaearon
Copy link
Collaborator

gaearon commented Feb 27, 2020

For fresh CRA, I just copy pasted the docs snippet and verified it still passes tests — whether I write ESM+Babel or CJS+require.

@NMinhNguyen
Copy link
Contributor Author

Cool, thanks! But be careful with assumptions about ESM - as far as I know, Jest doesn't support ESM (jestjs/jest#4842), so what it ends up importing is the CJS version anyway. In other words, it doesn't matter what syntax you use at the call site (in your tests).

@sebmarkbage
Copy link
Collaborator

@NMinhNguyen The syntax matters because using the import syntax adds a compatibility check at runtime that looks for esModule.

The problem is that we run Jest against both against source as it is. In this case Jest (Babel) is responsible for converting the export .... part to CommonJS. We also run Jest against an existing bundle and in that case Rollup built the bundle. We couldn't use just require(...) in our tests because that didn't add the interop layer that made it work with both.

However, now we won't run tests against the source any more. Just the bundle published to npm. So we don't need that compatibility anymore. So now we probably could revert the changes I made to the tests in #18145

@NMinhNguyen
Copy link
Contributor Author

NMinhNguyen commented Feb 27, 2020

@sebmarkbage sorry if I'm misunderstanding, but in CRA which points at the published react-shallow-renderer package, I think Jest would pick up the CJS bundle (also built with Rollup) which has no esModule (https://unpkg.com/browse/react-shallow-renderer@16.12.0/cjs/react-shallow-renderer.js), so the import syntax in the test shouldn't matter, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Shallow Renderer] Plan forward
5 participants