nytimes / kyt

Starting a new JS app? Build, test and run advanced apps with kyt 🔥
https://open.nytimes.com/introducing-kyt-our-web-app-configuration-toolkit-9ccddf6f6988
Other
1.92k stars 109 forks source link

feat(babel-preset-kyt-react): add babel-plugin-pure-static-props when productionTransforms is true #1075

Closed kohlmannj closed 2 years ago

kohlmannj commented 2 years ago

relates to DSYS-10

Description

This PR adds babel-plugin-pure-static-props to babel-preset-kyt-react's set of production transforms (enabled when the preset's productionTransforms option is set to true).

Motivation / Background

Static properties on [React] components break tree-shaking: https://github.com/styled-components/babel-plugin-styled-components/issues/245#issuecomment-525844957

This same commenter created babel-plugin-pure-static-props to address this. From its README:

This plugin replaces static property assignments on React components (e.g. MyComponent.defaultProps = {...}) with Object.assign() statements annotated with /*#__PURE__*/ comments so that tree-shaking will work correctly.

This is an especially important optimization for babel-preset-kyt-react because we add the static property displayName to every React component via babel-plugin-add-react-displayname. That is, this plugin is always enabled. (For that reason, we may consider always enabling babel-plugin-pure-static-props as well.)

kohlmannj commented 2 years ago

I took a moment to test this in the Monorepo. I found that nytimes/news#517 unconditionally sets useProductionTransforms: false for all Monorepo libraries that use babel-preset-kyt-react. I don't know why this was, and thus, I no longer want to tie babel-plugin-pure-static-props to it.

Therefore, I am going to revise this PR to unconditionally enable babel-plugin-pure-static-props, as previously hinted at in the original PR description.

Meanwhile, I feel confident that, in addition to more local testing, a corresponding Monorepo PR after merging and releasing this PR will be enough to tell us whether it's safe to unconditionally enable this plugin.

TL;DR — One change coming!

kohlmannj commented 2 years ago

Turns out I'm closing this PR. These test failures are representative of an issue I encountered with at least one source file in the Monorepo. I have no idea what to do about it, as the issue seems to lie within the plugin itself. And diagnosing that issue, while potentially valuable in the long term, moves this from a "quick fix" (just add plugin) to "potentially more involved fix."

Anyway, closing for now since unconditionally enabling babel-plugin-pure-static-props could cause build errors.