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

Bundle size #427

Closed
ritz078 opened this issue Oct 7, 2016 · 37 comments
Closed

Bundle size #427

ritz078 opened this issue Oct 7, 2016 · 37 comments

Comments

@ritz078
Copy link

ritz078 commented Oct 7, 2016

Hi @bvaughn ,

We are glad that this library is performing so well on mobile devices for us. One thing that we are concerned about is the bundle size. We are using webpack 2 for bundling with tree-shaking. Currently react-virtualized's size in our bundle can be seen below.

screen shot 2016-10-07 at 7 38 21 pm

This is when we are using InfiniteScroller with List. Even though I am fine with this, I would like to know whether this can be improved ? Because even slight reductions in file size can do wonders on mobile.

@bvaughn
Copy link
Owner

bvaughn commented Oct 7, 2016

Hi @ritz078. Nice to hear from you! 😄

That is a sweet looking breakdown. Help me better understand what it means though. For example, why does it show the dist/commonjs folder? Shouldn't you be using dist/es if you want the tree-shaking benefits of Webpack 2? (That's what the jsnext:main target points at.)

That should slim the module down a bit more for you as it wouldn't pull in any of the HOCs besides InfiniteScroller (which is small) or any of the Collection or Table related code.

@ritz078
Copy link
Author

ritz078 commented Oct 7, 2016

Yes you are right. it should have taken files from jsnext:main. I checked again and the files are loading from dist directory. Even checked the version of webpack in stats.json just to be sure 😄 . Seems to be an issue with webpack ? Either that's webpack issue or something in our configuration. will try to debug on our end and get back ASAP.

cc @TheLarkInn Can any error in configuration cause this error where the files are not taken from jsnext:main ?

webpack 2.1.0-beta.25

@TheLarkInn
Copy link

Only latest webpack pulls from the "module" field not jsnext:main. Otherwise you have to specify a seperate field in your config under resolver.modules

@bvaughn
Copy link
Owner

bvaughn commented Oct 7, 2016

You could also import from dist/es but that's a little janky.

import { List } from 'react-virtualized/dist/es';

@ritz078
Copy link
Author

ritz078 commented Oct 7, 2016

@bvaughn I guess that means "module" works for webpack 2 and "jsnext:main" for rollup.

@bvaughn
Copy link
Owner

bvaughn commented Oct 7, 2016

Oh I see! Seems related to webpack/webpack/issues/1979

I don't mind doing a quick bugfix release to add the "module" attribute to package.json if that's the preferred route now. I was unaware.

@ritz078
Copy link
Author

ritz078 commented Oct 7, 2016

was going through that PR only. Seems like even rollup is now supporting 'module'.

@bvaughn
Copy link
Owner

bvaughn commented Oct 7, 2016

@ritz078 give version 8.0.12 a quick try and let me know if the results are improved for you.

Also, thanks for the quick reply @TheLarkInn! Very helpful.

@bvaughn
Copy link
Owner

bvaughn commented Oct 7, 2016

Yeah, looks like I wasn't following along closely enough with the new lookup rules

@ritz078
Copy link
Author

ritz078 commented Oct 7, 2016

Here's the new contribution of react-virtualized.

screen shot 2016-10-08 at 3 36 43 am

but the chunk's size has increased by 18 KB. 😞

@bvaughn
Copy link
Owner

bvaughn commented Oct 7, 2016

Ok. That's looking better in terms of dist/es.

My next question would be- why do Table and Collection appear in the chart? If you're only using List then you should have List + Grid only. I don't see List (maybe because it's super small) but I do see Collection, Table, and CellMeasurer.

@ritz078
Copy link
Author

ritz078 commented Oct 7, 2016

yes that's my doubt too.

the code is simply

import { InfiniteLoader, AutoSizer, List } from 'react-virtualized'

@ritz078
Copy link
Author

ritz078 commented Oct 7, 2016

So it seems that tree shaking isn't working. If I change the above code to

import { InfiniteLoader } from 'react-virtualized/dist/es/InfiniteLoader'
import { AutoSizer } from 'react-virtualized/dist/es/AutoSizer'
import { List } from 'react-virtualized/dist/es/List'

file size change significantly. huge win this way.

screen shot 2016-10-08 at 4 22 17 am

cc: @TheLarkInn

@bvaughn
Copy link
Owner

bvaughn commented Oct 7, 2016

Maybe my dist/es/index.js is stupid?

That graph does indeed look much more reasonable.

Shouldn't you be able to at least change your imports to...

import { InfiniteLoader } from 'react-virtualized/InfiniteLoader'
import { AutoSizer } from 'react-virtualized/AutoSizer'
import { List } from 'react-virtualized/List'

...given the module lookup rules?

@ritz078
Copy link
Author

ritz078 commented Oct 8, 2016

Its not working that way. Even individually picking es build was giving error during execution. Finally I am loading commonjs versions individually.

@bvaughn
Copy link
Owner

bvaughn commented Oct 8, 2016

Could you elaborate a bit more @ritz078? What wasn't working about the es build?

@ritz078
Copy link
Author

ritz078 commented Oct 8, 2016

Here are all the cases that I tried

Case I

import {InfiniteLoader, AutoSizer, List} from 'react-virtualized'

Build: ✅
Loading form : es
File size: increases
Tree shaking: ❌

Case II

import { InfiniteLoader } from 'react-virtualized/InfiniteLoader'
import { AutoSizer } from 'react-virtualized/AutoSizer'
import { List } from 'react-virtualized/List'

Build: ❌ can't resolve modules

Case III

import { InfiniteLoader } from 'react-virtualized/dist/es/InfiniteLoader'
import { AutoSizer } from 'react-virtualized/dist/es/AutoSizer'
import { List } from 'react-virtualized/dist/es/List'

Build: ✅
File size: decreases
But when I make a request, it gives an error "Unhandled Promise rejection: SyntaxError: Unexpected token import". This has nothing to do with react-virtualized. We even tried this with react-router and got the same error. So either webpack issue or our config. Its really tough with the lack of docs on webpack 2 currently.

Case IV

import { InfiniteLoader } from 'react-virtualized/dist/commonjs/InfiniteLoader'
import { AutoSizer } from 'react-virtualized/dist/commonjs/AutoSizer'
import { List } from 'react-virtualized/dist/commonjs/List'

Build: ✅
File size : decreases
Everything works fine.

So currently we are using this. Hopefully we will be able to figure out the webpack issue soon.

@bvaughn
Copy link
Owner

bvaughn commented Oct 8, 2016

I see. The error you mentioned ("Unhandled Promise rejection: SyntaxError: Unexpected token import") sounds like Webpack is configured not to transpile modules within the node_modules folder- which is normally a reasonable setting, although in this case... it doesn't make sense. Not sure what the appropriate approach is w.r.t. es modules.

@ritz078
Copy link
Author

ritz078 commented Oct 9, 2016

Hopefully we will be able to figure that out soon. Closing this issue. Thanks for the help.

@ritz078 ritz078 closed this as completed Oct 9, 2016
@bvaughn
Copy link
Owner

bvaughn commented Oct 9, 2016

Any time @ritz078!

Please share any additional findings if you think of it. 😁

@ritz078
Copy link
Author

ritz078 commented Oct 9, 2016

Surely.

@SpencerCarstens
Copy link

Ran into the same exact issue. Webpack 2 and tree-shaking did not work until:

// Changing...
import { AutoSizer, Column, Table, InfiniteLoader } from 'react-virtualized';

// To...
import { InfiniteLoader } from 'react-virtualized/dist/es/InfiniteLoader';
import { AutoSizer } from 'react-virtualized/dist/es/AutoSizer';
import { Column, Table } from 'react-virtualized/dist/es/Table';

👍

@rt2zz
Copy link

rt2zz commented Feb 14, 2017

thanks for the solutions, works for my setup

found it https://alexkuz.github.io/webpack-chart/

@rubencodes
Copy link

Same, halved the size of my react-virtualized imports. Thanks for sharing!

@romulof
Copy link
Contributor

romulof commented May 19, 2017

I've done some investigation on this same problem today.
Here are my findings on why importing everything from index bugs webpack tree shaking:

  1. The syntax export NS from/export default from still a proposal. It ain't ES6/ES2015 standard.
  2. The most simple way to transform this notation is calling a standard import then export the imported module.
  3. webpack2 Tree Shaking works by removing the export statement, so the import statement is kept.

Take a look at webpack output for my project (without mangling and keeping the comments):

/* exports provided: ArrowKeyStepper, AutoSizer, CellMeasurer, CellMeasurerCache, Collection, ColumnSizer, accessibilityOverscanIndicesGetter, defaultCellRangeRenderer, defaultOverscanIndicesGetter, Grid, InfiniteLoader, List, createMasonryCellPositioner, Masonry, MultiGrid, ScrollSync, defaultTableCellDataGetter, defaultTableCellRenderer, defaultTableHeaderRenderer, defaultTableHeaderRowRenderer, defaultTableRowRenderer, Table, Column, SortDirection, SortIndicator, WindowScroller */
/* exports used: CellMeasurerCache, CellMeasurer, AutoSizer, InfiniteLoader, List */
/*!**********************************************!*\
  !*** ./~/react-virtualized/dist/es/index.js ***!
  \**********************************************/
/***/
function(module, __webpack_exports__, __webpack_require__) {
    "use strict";
    /* harmony import */
    var __WEBPACK_IMPORTED_MODULE_1__AutoSizer__ = (__webpack_require__(/*! ./ArrowKeyStepper */ 847), 
    __webpack_require__(/*! ./AutoSizer */ 848));
    /* harmony reexport (binding) */
    __webpack_require__.d(__webpack_exports__, "c", function() {
        return __WEBPACK_IMPORTED_MODULE_1__AutoSizer__.a;
    });
    /* harmony import */
    var __WEBPACK_IMPORTED_MODULE_2__CellMeasurer__ = __webpack_require__(/*! ./CellMeasurer */ 353);
    /* harmony reexport (binding) */
    __webpack_require__.d(__webpack_exports__, "b", function() {
        return __WEBPACK_IMPORTED_MODULE_2__CellMeasurer__.a;
    }), /* harmony reexport (binding) */
    __webpack_require__.d(__webpack_exports__, "a", function() {
        return __WEBPACK_IMPORTED_MODULE_2__CellMeasurer__.b;
    });
    /* harmony import */
    var __WEBPACK_IMPORTED_MODULE_6__InfiniteLoader__ = (__webpack_require__(/*! ./Collection */ 853), 
    __webpack_require__(/*! ./ColumnSizer */ 855), __webpack_require__(/*! ./Grid */ 147), 
    __webpack_require__(/*! ./InfiniteLoader */ 861));
    /* harmony reexport (binding) */
    __webpack_require__.d(__webpack_exports__, "d", function() {
        return __WEBPACK_IMPORTED_MODULE_6__InfiniteLoader__.a;
    });
    /* harmony import */
    var __WEBPACK_IMPORTED_MODULE_7__List__ = __webpack_require__(/*! ./List */ 862);
    /* harmony reexport (binding) */
    __webpack_require__.d(__webpack_exports__, "e", function() {
        return __WEBPACK_IMPORTED_MODULE_7__List__.a;
    });
    /* harmony import */
    __webpack_require__(/*! ./Masonry */ 865), __webpack_require__(/*! ./MultiGrid */ 867), 
    __webpack_require__(/*! ./ScrollSync */ 868), __webpack_require__(/*! ./Table */ 869), 
    __webpack_require__(/*! ./WindowScroller */ 870);
}, /* 873 */

The comments on the first 2 lines shows exactly that tree shaking algorithm is working, and you can see at the end of the module that the harmony imports of the unused modules are still there, but no exports.

I tried removing babel-plugin-transform-export-extensions to deliver the whole export ns from syntax to webpack, allowing it to properly manage the unused export {} from, but It didn't change the result.

My conclusion is that this is actually a webpack issue or an unimplemented feature. For now it's better to use direct imports like import { List } from 'react-virtualized/dist/es/List'.

Right now I'm trying to find something related on webpack issues.

@drKnoxy
Copy link

drKnoxy commented Oct 2, 2017

@TheLarkInn any chance you could weigh in on where the issue should be corrected; webpack / library

The issue with the es index.js file's export syntax doesn't seem to allow for tree shaking

export { ArrowKeyStepper } from "./ArrowKeyStepper";
export { AutoSizer } from "./AutoSizer";
export { CellMeasurer, CellMeasurerCache } from "./CellMeasurer";
export { Collection } from "./Collection";
export { ColumnSizer } from "./ColumnSizer";
export { accessibilityOverscanIndicesGetter, defaultCellRangeRenderer, defaultOverscanIndicesGetter, Grid } from "./Grid";
export { InfiniteLoader } from "./InfiniteLoader";
export { List } from "./List";
export { createCellPositioner as createMasonryCellPositioner, Masonry } from "./Masonry";
export { MultiGrid } from "./MultiGrid";
export { ScrollSync } from "./ScrollSync";
export { defaultCellDataGetter as defaultTableCellDataGetter, defaultCellRenderer as defaultTableCellRenderer, defaultHeaderRenderer as defaultTableHeaderRenderer, defaultHeaderRowRenderer as defaultTableHeaderRowRenderer, defaultRowRenderer as defaultTableRowRenderer, Table, Column, SortDirection, SortIndicator } from "./Table";
export { WindowScroller } from "./WindowScroller";

@TrySound
Copy link
Contributor

TrySound commented Oct 2, 2017

@drKnoxy The problem is untreeshakable classes which are transpilled to function constructor with prototype. It's side effect for bundlers. All component based libraries affected by this. Just use paths like import AutoSizer from 'react-virtualized/dist/es/AutoSizer';. In the future side-effects field may fix this.

@drKnoxy
Copy link

drKnoxy commented Oct 3, 2017 via email

@TrySound
Copy link
Contributor

TrySound commented Oct 3, 2017

It works, but it's not panacea. Static analyzing is hard when you deal with javascript.

@alexparish
Copy link

alexparish commented May 17, 2018

@TrySound Now the side-effects field has been added to package.json should it be possible to use the following import and expect webpack 4 to tree-shake the imports?

import { InfiniteLoader, AutoSizer, List } from 'react-virtualized'

I tried this but my bundle did not decrease in size.

@TrySound
Copy link
Contributor

@alexparish That sucks. I will make this library treeshakable for v10. sideEffects is not the only way to achieve this.

@alexparish
Copy link

alexparish commented May 18, 2018

@TrySound That would be great.

I believe the only impediment to tree-shaking the raw source code of this library is caused by the addition of the babel property in package.json.

Having added jsnext:main to the mainFields property of my webpack config in order to pick up the source from dist/es/index.js, Babel is trying to find ./.babelrc.js in the project but failing:

Module build failed: Error: Couldn't find preset "./.babelrc.js" relative to directory "/Users/Projects/project-name-redacted/node_modules/react-virtualized"
    at Array.map (<anonymous>)

Out of interest, for v10 would you make the commonjs output tree-shakable? Is that even possible?

@TrySound
Copy link
Contributor

@alexparish ignore node_modules

{
  test: /\.js$/,
  exclude: /node_modules/,
  loader: 'babel-loader'
}

@TrySound
Copy link
Contributor

And no, commonjs is not treeshakable. However rollup plugin commonjs may on some cases convert to treeshakable esm.

@miguelocarvajal
Copy link

@TrySound Ignoring node_modules only works for Webpack.

I am having the same issue with Metro Bundler where I get the Couldn't find preset "./.babelrc.js" message. I think the best thing would be to strip that config from package.json when react-virtualized is built, not sure how to go about doing that.

Any thoughts?

@TrySound
Copy link
Contributor

TrySound commented Jun 1, 2018

@miguelocarvajal If there is such problem you probably overtranspile your vendors. Is it fast?

@miguelocarvajal
Copy link

@TrySound Fast as it has ever been :)

To make sure its not my configuration I just tried creating a new project with react-native and I still get the same message. These are the steps I followed:

  1. react-native init VirtualizedTest
  2. yarn add react-virtualized
  3. Modified App.js to include an empty InfiniteLoader
  4. react-native run-ios

Just with those steps and now adding any other dependencies the app no longer runs due to that babel config.

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

No branches or pull requests