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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tree shaking does not work with Material UI #1984

Closed
alex996 opened this issue Sep 5, 2018 · 14 comments 路 Fixed by #2993
Closed

Tree shaking does not work with Material UI #1984

alex996 opened this issue Sep 5, 2018 · 14 comments 路 Fixed by #2993

Comments

@alex996
Copy link

alex996 commented Sep 5, 2018

馃悰 bug report

When using --experimental-scope-hoisting with @material-ui/core, the bundle builds with no errors. However, when the app is served from index.html, its fails with ReferenceError: React is not defined. Tried both CJS and ESM, same error. When the flag is not used, it builds and runs fine. I also ran into a similar issue with @material-ui/icons; the error is TypeError: (0 , Oj.default) is not a function. See code samples and metrics in the gist.

馃帥 Configuration (.babelrc, package.json)

{
  "presets": ["env", "react"]
}
"scripts": {
  "build": "rm -rf dist && parcel build index.html",
  "build:shake": "rm -rf dist && parcel build --experimental-scope-hoisting index.html",
  "serve:dist": "serve dist"
}

馃 Expected Behavior

It should render with no JavaScript errors in the console.

馃槸 Current Behavior

It fails to render with Uncaught ReferenceError: React is not defined. Blank screen, nothing is displayed.

馃敠 Context

I was curious to see if Parcel 1.9.x supports tree shaking with Material UI. With Webpack, tree shaking doesn't seem to work for CJS, but does work with ESM (which MUI exports for evergreen browsers). fuse-box produced heavy bundle size (maybe I was using it wrong with Babel); I'm yet to try out with Rollup.

馃捇 Code Sample

// index.js
import React from 'react'
import { render } from 'react-dom'
import Typography from '@material-ui/core/Typography'
// or import { Typography } from '@material-ui/core'

render(
  <Typography>Hello</Typography>,
  document.getElementById('app')
)

馃憠 Please see the gist for details. 馃憟

馃實 Your Environment

Software Version(s)
Parcel 1.9.6
Node 10.6.0
npm/Yarn 1.7.0
Operating System Ubuntu 18.04

P.S. I understand it's an experimental feature; just wanted to bring up a real word scenario where it falls short :-) Thanks

@oliviertassinari
Copy link

I confirm the issue. I can reproduce it too.

@mischnic
Copy link
Member

With tree shaking enabled, I only get this error:

Uncaught TypeError: (0 , $qiuk$var$_deepmerge.default) is not a function
    at $qiuk$var$createPalette (material.10d89c23.js:12721)
    at $Wq15$var$createMuiTheme (material.10d89c23.js:13150)
    at $XQXT$var$getDefaultTheme (material.10d89c23.js:13409)
    at material.10d89c23.js:13417
    at material.10d89c23.js:14170

@devongovett
Copy link
Member

Can someone verify this in the latest version of Parcel?

@oliviertassinari
Copy link

With the latest version of Material-UI (v4.0.0-alpha). We have changed the tree shaking strategy.

@mischnic
Copy link
Member

mischnic commented Mar 6, 2019

import { Typography } from '@material-ui/core'

console.log(Typography)

trying to build with scope hoisting:

馃毃  Property right of AssignmentExpression expected node to be of a type ["Expression"] but instead got null
    at Object.validate (.../parcel/node_modules/@babel/types/lib/definitions/utils.js:128:13)
    at Object.validate (.../parcel/node_modules/@babel/types/lib/validators/validate.js:17:9)
    at NodePath._replaceWith (.../parcel/node_modules/@babel/traverse/lib/path/replacement.js:194:9)
    at NodePath._remove (.../parcel/node_modules/@babel/traverse/lib/path/removal.js:51:10)
    at NodePath.remove (.../parcel/node_modules/@babel/traverse/lib/path/removal.js:30:8)
    at remove (.../parcel/packages/core/parcel-bundler/src/scope-hoisting/shake.js:109:25)
    at Array.forEach (<anonymous>)
    at Object.keys.forEach.name (.../parcel/packages/core/parcel-bundler/src/scope-hoisting/shake.js:30:65)
    at Array.forEach (<anonymous>)
    at treeShake (.../parcel/packages/core/parcel-bundler/src/scope-hoisting/shake.js:20:33)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Commenting out the treeShake function in concat.js (-> #2729) leads to the error below


import React from 'react'
import { render } from 'react-dom'
import Typography from '@material-ui/core/Typography'

render(
  <Typography>Hello</Typography>,
  document.getElementById('app')
)

->

1984-material.201e74bf.js:4310 TypeError: (0 , $bJxL$var$_hyphenateStyleName2.default) is not a function
    at $bJxL$var$convertCase (1984-material.201e74bf.js:11895)
    at Array.onProcessStyle (1984-material.201e74bf.js:11922)
    at PluginsRegistry.onProcessStyle (1984-material.201e74bf.js:9942)
    at PluginsRegistry.onProcessRule (1984-material.201e74bf.js:9929)
    at Array.forEach (<anonymous>)
    at RuleList.process (1984-material.201e74bf.js:9416)
    at new StyleSheet (1984-material.201e74bf.js:9606)
    at Jss.createStyleSheet (1984-material.201e74bf.js:11301)
    at WithStyles.createSheet (1984-material.201e74bf.js:13958)
    at WithStyles.attach (1984-material.201e74bf.js:13927)
1984-material.201e74bf.js:11895 Uncaught TypeError: (0 , $bJxL$var$_hyphenateStyleName2.default) is not a function
    at $bJxL$var$convertCase (1984-material.201e74bf.js:11895)
    at Array.onProcessStyle (1984-material.201e74bf.js:11922)
    at PluginsRegistry.onProcessStyle (1984-material.201e74bf.js:9942)
    at PluginsRegistry.onProcessRule (1984-material.201e74bf.js:9929)
    at Array.forEach (<anonymous>)
    at RuleList.process (1984-material.201e74bf.js:9416)
    at new StyleSheet (1984-material.201e74bf.js:9606)
    at Jss.createStyleSheet (1984-material.201e74bf.js:11301)
    at WithStyles.createSheet (1984-material.201e74bf.js:13958)
    at WithStyles.attach (1984-material.201e74bf.js:13927)

@mischnic
Copy link
Member

mischnic commented Mar 6, 2019

(The corresponding code is all in transitive dependencies)

@devongovett This is caused is by this bit (/node_modules/jss-camel-case/lib/index.js):

var _hyphenateStyleName = require('hyphenate-style-name');
var _hyphenateStyleName2 = _interopRequireDefault(_hyphenateStyleName);

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { 'default': obj }; }

// ...

(0, _hyphenateStyleName2['default'])(prop)

The module entry of hyphenate-style-name is used (which contains export default ...). _hyphenateStyleName is then just {default: ...} (without __esModule), causing _interopRequireDefault to wrap it again and making it {default: {default: ...}}, resulting in the failing function call.
Not sure where to handle this...

@oliviertassinari
Copy link

oliviertassinari commented Mar 6, 2019

cc @kof (just for context)

@gisderdube
Copy link

Also having this issue, I was a bit surprised having 5MB increased bundle size for two icons 馃槀

@oliviertassinari
Copy link

@gisderdube Does it work better with v4-alpha?

@gisderdube
Copy link

Didn't check with v4 alpha, but with --experimental-scope-hoisting it seems to remove all of the other icons in the build. However for development, they are still included. I went on to remove the material-ui package altogether.

@devongovett
Copy link
Member

devongovett commented May 7, 2019

Related to #2977. I made a commit to the Parcel 2 tree shaking branch that fixes this: ab98afe. It will need to be back ported.

@devongovett
Copy link
Member

Should be fixed by #2993.

@mattrossman
Copy link

Sorry for pinging this old issue, but tree shaking is still not working with Material UI in Parcel 2 for me. As soon as I do something like import { Button } from '@material-ui/core, I can see in parcel-bundle-reports that the entire Material UI core is included, while import Button from '@material-ui/core/Button works as expected.

Is there any CLI flag or extra configuration that we're supposed to be doing in v2?

@mischnic
Copy link
Member

Please follow #4565, I'm currently working on fixing that in this PR: #4861

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.

7 participants