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

Only use Rollup's CommonJS plugin for "react-art" #18186

Merged
merged 2 commits into from
Feb 29, 2020

Conversation

sebmarkbage
Copy link
Collaborator

This upgrades the shallow renderer dep, and properly make react-debug-tools and jest-react depend on object-assign as an external dependency instead of compiling it in.

This means that "art" is our only CJS dependency that needs to be compiled with Rollup. So we can now disable the CJS plugin for everything else. That ensures we don't reintroduce new CJS dependencies.

I'm hoping to port "art" to ESM or we can just drop the UMD build. Then we can get rid of the plugin completely.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 29, 2020
…ernal dep

This avoids it being compiled into the output.
We still need it for the "art" UMD builds but nothing else should have
CommonJS dependencies anymore.
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 29, 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 598f445:

Sandbox Source
crazy-nobel-7tud8 Configuration

@sebmarkbage
Copy link
Collaborator Author

@NMinhNguyen I merged yours and rebased mine.

@NMinhNguyen
Copy link
Contributor

NMinhNguyen commented Feb 29, 2020

I actually realised something.. I remember seeing an issue in hoist-non-react-statics mridgway/hoist-non-react-statics#52 where it used to be published with module field and it broke another dependency that was doing require('hoist-non-react-statics') (https://github.com/kentor/react-click-outside/blob/e3e1337/index.js#L3) because webpack automatically favours ESM. I wonder if I should update the forwarding module

module.exports = require('react-shallow-renderer');
to have a .default check? Perhaps there's some CJS setup somewhere that uses webpack (e.g. to serve bundles in the browser via Karma), and it could break. Didn't want to create a separate issue for this (kinda need more time to repro as well), but thought I'd mention it somewhere.

@sizebot
Copy link

sizebot commented Feb 29, 2020

Details of bundled changes.

Comparing: c26506a...598f445

jest-react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
jest-react.development.js -33.8% -35.3% 5.95 KB 3.93 KB 2.13 KB 1.38 KB NODE_DEV
jest-react.production.min.js -34.0% -28.5% 2.5 KB 1.65 KB 1.26 KB 926 B NODE_PROD

react-debug-tools

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-debug-tools.development.js -9.9% -12.4% 20.28 KB 18.27 KB 5.85 KB 5.13 KB NODE_DEV
react-debug-tools.production.min.js -13.7% -15.1% 6.24 KB 5.38 KB 2.45 KB 2.08 KB NODE_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against 7903e43

@sizebot
Copy link

sizebot commented Feb 29, 2020

Details of bundled changes.

Comparing: c26506a...598f445

react-debug-tools

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-debug-tools.development.js -9.9% -12.5% 20.27 KB 18.26 KB 5.84 KB 5.12 KB NODE_DEV
react-debug-tools.production.min.js -13.7% -15.1% 6.23 KB 5.37 KB 2.44 KB 2.07 KB NODE_PROD

jest-react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
jest-react.development.js -33.9% -35.4% 5.93 KB 3.92 KB 2.12 KB 1.37 KB NODE_DEV
jest-react.production.min.js -34.2% -28.5% 2.49 KB 1.64 KB 1.25 KB 918 B NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against 7903e43

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.

5 participants