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

[docs] Update minimizing-bundle-size.md #7169

Merged
merged 3 commits into from Jun 19, 2017
Merged

[docs] Update minimizing-bundle-size.md #7169

merged 3 commits into from Jun 19, 2017

Conversation

kazazor
Copy link
Contributor

@kazazor kazazor commented Jun 17, 2017

  • PR has tests / docs demo, and is linted.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 17, 2017

Hey, thanks for submitting that PR! We have an open issue for that point #5625.
I think this point worth a sub section in that page of the documentation.
I'm not 100% sure this is the best solution available. Maybe @umidbekkarimov can guide us regarding how we should document that point. I believe that we can close the linked issue once it's well documented :).

@oliviertassinari oliviertassinari changed the title Update minimizing-bundle-size.md [docs] Update minimizing-bundle-size.md Jun 17, 2017
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation v1.x.x labels Jun 17, 2017
@avocadowastaken
Copy link
Contributor

Any babel plugin (including babel-plugin-material-ui) should be a short term workaround, tree shaking should be a solution. And it works. It disables imports and makes components unused, but uglifyjs doesn't count them as dead code. Mostly it's because of propTypes and another statics in classes.

Try to uglify this code (https://skalman.github.io/UglifyJS-online/)

var Component = (function() {
   // Unused component without any statics.
   function Foo () {}
 
   // Unused component with prop types.
   function Bar() {}
   Bar.propTypes = { children: PropTypes.node }

   // Used component.
   function Baz() {}

   return Baz;
})();

See this issue babel/babel#5632 for more details.

@kazazor
Copy link
Contributor Author

kazazor commented Jun 18, 2017

I agree tree shaking is the right solution looking forward. But this Doc doesn't say anything about it. For example tree shaking does work in Rollup.js

I will edit the page to include also tree shaking information

@kazazor
Copy link
Contributor Author

kazazor commented Jun 18, 2017

@umidbekkarimov - I committed a new proposal that IMO gives the users the most information about the different options to optimize the bundle. Please review it and let me know what you think :)

@avocadowastaken
Copy link
Contributor

@kazazor thank you for your time 👍. I have no problems with this.

But as @oliviertassinari mentioned, it's not the only one solution, also another babel plugins could be mentioned.
e.g:

babel-lodash-plugin: #5625 (comment)
babel-plugin-import: #5625 (comment)

But I didn't use them, so I can't help you any further 😓.

P.S. just checked micro example in rollup and it's surprisingly works.

@avocadowastaken
Copy link
Contributor

Oh, just notice that it's for next branch, I didn't test babel-plugin-material-ui with material-ui@1 yet, it's awesome if it works out of the box with next 😄.

@kazazor
Copy link
Contributor Author

kazazor commented Jun 18, 2017

I actually used the two plugins you mentioned, and it works :)
I did not mention it in this doc cause I assumed we're only talking here about optimizing the material-ui imports.. If this is a general optimizations tricks I have plenty more, just think it's not the job of material UI to specify all the ones that doesn't related to itself :)

Please let me double check babel-plugin-material-ui and I'll comment here if we can merge it or not 👯‍♂️

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 18, 2017

@kazazor I much prefer the new version :).

I did not mention it in this doc cause I assumed we're only talking here about optimizing the material-ui imports.. If this is a general optimizations tricks I have plenty more, just think it's not the job of material UI to specify all the ones that doesn't related to itself :)

@umidbekkarimov Right, thanks for linking the two other plugins!
I think that we can link the 3 babel plugins. They have different tradeoffs, I would rather make the documentation less opinionated.

[
  {
    "libraryName": "antd",
    "libraryDirectory": "lib",
    "style": true
  },
  {
    "libraryName": "material-ui",
    "libraryDirectory": "components",
    "camel2DashComponentName": false
  }
]

@avocadowastaken
Copy link
Contributor

avocadowastaken commented Jun 18, 2017

Just published next version (https://github.com/umidbekkarimov/babel-plugin-material-ui/tree/next).

Had to replace regexp lookup with babylon parser (https://github.com/umidbekkarimov/babel-plugin-material-ui/blob/next/src/utils.js#L34).

So basically it works now (all tests passed, test PRs are welcomed), but without support of material-ui/svg-icons, I guess it's have own package now.


P.S. maybe I should add few changes and make it index-file shape agnostic, but it would be hard coded smarter analogue of babel-plugin-import.

@kazazor
Copy link
Contributor Author

kazazor commented Jun 18, 2017

@oliviertassinari so I didn't understand if you want me to also add lodash plugin? :)

Also regarding the import plugin, isn't that what the material-ui plugin brings you..? Why should we use both?

@oliviertassinari
Copy link
Member

@umidbekkarimov I'm happy to hear that babel-plugin-material-ui is supporting the next branch :).

so I didn't understand

@kazazor I think that it would be good to document the three babel alternatives available as they have different tradeoffs. They are not supposed to be used at the same time, people will have to pick one if that's something they are interested in. I would rather let the community decide what's the best solution for them.

@oliviertassinari
Copy link
Member

@kazazor I'm merging, thanks for the update.

@oliviertassinari oliviertassinari merged commit d6757e3 into mui:next Jun 19, 2017
@kazazor kazazor deleted the patch-1 branch June 19, 2017 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants