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

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

Closed
wants to merge 2 commits into from

Conversation

kohlmannj
Copy link

@kohlmannj kohlmannj commented Sep 28, 2022

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: styled-components/babel-plugin-styled-components#245 (comment)

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
Copy link
Author

kohlmannj commented Sep 28, 2022

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
Copy link
Author

kohlmannj commented Sep 29, 2022

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.

@kohlmannj kohlmannj closed this Sep 29, 2022
@kohlmannj kohlmannj deleted the dsys-10 branch September 29, 2022 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants