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

Static properties on components break tree-shaking #245

Open
mbrowne opened this issue Aug 28, 2019 · 11 comments · May be fixed by #248
Open

Static properties on components break tree-shaking #245

mbrowne opened this issue Aug 28, 2019 · 11 comments · May be fixed by #248

Comments

@mbrowne
Copy link
Contributor

mbrowne commented Aug 28, 2019

In our app we are finding that any time a component that uses styled-components has a static property added to it, that component is always included in the bundle even if it's not imported, i.e. tree shaking does not work. For example:

const Wrapper = styled.div`
    color: blue;
`

function MyComponent() {
    return <Wrapper>hi</Wrapper>
}
// this breaks tree-shaking
MyComponent.displayName = 'Foo'

In some cases we're able to remove the displayName or use default function argument values instead of defaultProps, but there are other cases where we do need to use these properties.

This seems to be caused by the way terser (or uglify-js) removes dead code and isn't really an issue in babel-plugin-styled-components per se, but it would be great if babel-plugin-styled-components could include an option to address this.

@mbrowne
Copy link
Contributor Author

mbrowne commented Aug 28, 2019

UPDATE 8/30/19:
It turns out that I was using the PURE annotations incorrectly (as explained to me here). I updated my Babel plugin to use an IIFE to wrap the static properties instead:
https://github.com/mbrowne/babel-plugin-pure-static-props

Example output:

const MyComponent =
/*#__PURE__*/
function () {
  function MyComponent() {
    return _react.default.createElement(Wrapper);
  }
  MyComponent.displayName = 'Foo';
  return FunctionComponent;
}();

Here are some more details:

As with styled itself, it seems the best way to correct this problem is to add /*#__PURE__*/ comments to the code. (I suppose uglify-js must be worried that setting a static property could have side effects, e.g. if that property had a setter function.) Unfortunately, 'pure' annotations seem to be ignored for assignments and only work for function calls. But changing it to Object.assign() works:

/*#__PURE__*/
Object.assign(MyComponent, { displayName: 'Foo' })

I wrote a Babel plugin to test this out and verify that it works:
https://github.com/mbrowne/babel-plugin-pure-static-props

I tested it on our app, but didn't write unit tests for it yet because I wanted to share it here first...while it would be possible to solve this with a separate Babel plugin, I think it would be better if this functionality were part of babel-plugin-styled-components. BTW, my plugin isn't actually checking that styled-components is used; it just applies this transformation to all React components since I figured there was no reason not to (perhaps there are other edge cases that would also cause static properties to break tree-shaking).

Also, I did run into an issue with Rollup... Let me back up first and explain that we have a library for reusable UI components that we're importing from our app. We want the tree-shaking to happen in our app so that if we import only some of the library components, only those components are included in the bundle. We are building the library with rollup. The problem is that by default, rollup completely removes the Object.assign() statements from the code, e.g. it would remove this line:

/*#__PURE__*/
Object.assign(MyComponent, { displayName: 'Foo' })

There is a workaround though: rollup allows you to turn off this feature by adding this to the config:

    treeshake: {
        annotations: false,
    },

(Or you can pass --no-treeshake.annotations at the command line.)

@mxstbr
Copy link
Member

mxstbr commented Sep 9, 2019

This is very interesting, thank you for digging into this! 🙏

A PR that makes adds the IIFE wrapper (next to the PURE comment) when setting pure: true would be much appreciated, I definitely think the Babel plugin should be doing whatever it can to make tree shaking possible.

@mbrowne
Copy link
Contributor Author

mbrowne commented Sep 9, 2019

Great, glad you are open to including this. I can probably work on a PR for this sometime in the next week or two.

@mxstbr
Copy link
Member

mxstbr commented Sep 9, 2019

Amazing, can't wait @mbrowne! 🙏

@mbrowne
Copy link
Contributor Author

mbrowne commented Sep 10, 2019

For anyone who's interested, I created a test repo so that others can easily reproduce this issue, and also so that we have something to test against to ensure that my PR (when it's ready) covers all the bases:
https://github.com/mbrowne/CRA-error-template/tree/styled-components-tree-shaking
(be sure to check out this specific branch)

If you run yarn build on my demo, you can see that only Component1 tree-shakes correctly. (Earlier I thought that even that would break, because it seemed like this issue was affecting all of our library components, but fortunately in that simple case it's already working correctly.)

mbrowne added a commit to mbrowne/babel-plugin-styled-components that referenced this issue Sep 24, 2019
mbrowne added a commit to mbrowne/babel-plugin-styled-components that referenced this issue Sep 24, 2019
mbrowne added a commit to mbrowne/babel-plugin-styled-components that referenced this issue Sep 24, 2019
mbrowne added a commit to mbrowne/babel-plugin-styled-components that referenced this issue Sep 25, 2019
mbrowne added a commit to mbrowne/babel-plugin-styled-components that referenced this issue Sep 25, 2019
mbrowne added a commit to mbrowne/babel-plugin-styled-components that referenced this issue Sep 25, 2019
@mbrowne mbrowne linked a pull request Sep 26, 2019 that will close this issue
@ammmze
Copy link

ammmze commented Feb 25, 2021

Is this issue actually specific to styled-components? Maybe i've got something goofed in my project i'm in currently, but regardless of using styled-components, if I add a static property (MyComponent.displayName = 'MyComponent') to the function component, then it no longer gets removed during dead code removal. But if I remove the static property then it gets removed. This was also the case prior to me adding babel-plugin-styled-comonents to my project.

Edit: Maybe its because my project is using webpack instead of rollup. Sounds like rollup is able to detect the static properties on function components, but webpack (well technically uglifyjs or terser) doesn't 🤷🏻‍♂️

@mbrowne
Copy link
Contributor Author

mbrowne commented Feb 25, 2021

@ammmze It isn't only an issue for styled-components. But in order to address it in a different plugin, that plugin would have to be aware of how to detect styled components, and duplicate all that detection logic that's already included in babel-plugin-styled-components. Also, babel-plugin-styled-components already adds some PURE annotations in order to make tree-shaking work correctly, so it's logical that it should also handle the static properties case (displayName, etc).

To elaborate a bit on detecting a styled component: it can't be done the same way as for a regular React component. Babel plugins analyze the source code only, so detection methods used to detect React components used by other Babel plugins (like babel-plugin-transform-react-remove-prop-types, to give one example) wouldn't work here.

@ammmze
Copy link

ammmze commented Feb 26, 2021

Gotcha! So it looks like with the fix you have in the PR, it only seems to take effect for components are literally made from styled components (i.e. export const Foo = styled.div`color: blue`;) or functions that return styled components. Do you have any tips for function components that just return native react elements? Something like below doesn't seem to get wrapped like styled-components and functions that return styled-components.

const Foo = (props) => (<div {...props}>Foo</div>);
Foo.displayName = 'Foo';
export default Foo;

@mbrowne
Copy link
Contributor Author

mbrowne commented Feb 26, 2021

@ammmze Yeah, I had forgotten about that until someone pointed it out to me recently. They told me they're now using both my PR to this repo and my older plugin, babel-plugin-pure-static-props. I still think that all the use cases related to styled-components should be handled by babel-plugin-styled-components, and that other plugins shouldn't be concerning themselves with detecting styled components. But for other use cases, I'm thinking maybe I should un-deprecate babel-plugin-pure-static-props and update it with the efficiency improvements I made as part of this PR.

@ammmze
Copy link

ammmze commented Feb 26, 2021

👍🏻 That is what I'm thinking. I think it makes perfect sense for the babel-plugin-styled-components to apply it to any component is created from styled components (i.e. const Foo = styled.div`color: blue`;), and functions that return styled-components...anything related to styled-components. But for vanilla I think it should be a separate plugin. I did try adding your deprecated plugin, but ran into some issues.

It blew up when it hit an old class component (really we have a bunch of them...this was just the first one it hit) we have in our stuff...

export default class Checkbox extends Component {
    //...
}

It gave me the following:

Property declaration of ExportDefaultDeclaration expected node to be of a type ["FunctionDeclaration","TSDeclareFunction","ClassDeclaration","Expression"] but instead got "VariableDeclaration"

But I can dig into that and report in your repo.

@ammmze
Copy link

ammmze commented Feb 26, 2021

@mbrowne FYI i've forked your project and i'm setting up test fixtures to cover some various use cases and I'll submit a PR when complete

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

Successfully merging a pull request may close this issue.

3 participants