renatorib / react-powerplug

:electric_plug: Renderless Containers
https://renatorib.github.io/react-powerplug
MIT License
2.68k stars 101 forks source link

implement defense of dot js-style poly package structure? #141

Closed jedwards1211 closed 6 years ago

jedwards1211 commented 6 years ago

Hi guys! I recently learned a bunch of stuff about how to reduce webpack bundle size while supporting SSR, and I've been making PRs to some of my favorite packages to support this (just got one merged in redux-form: https://github.com/erikras/redux-form/pull/4123)

Would you guys be open to a PR for the same here? I admit I don't know as much about the relative advantages of the rollup build you guys are using, but I believe that the approach outlined below would enable people to save more KB in their Webpack bundle.

Basically the package structure is like this, as recommended by the In Defense of .js proposal:

package.json

{
  "main": "./index.js",
  "modules.root": "./es",
  "sideEffects": false
}

Package structure

With this setup, import State from 'react-powerplug/State' works equally well in node and webpack, resolving to:

Environment Path
Node node_modules/react-powerplug/State.js
Webpack node_modules/react-powerplug/es/State.js

Additionally, even with Webpack tree shaking, there is less overhead with import State from 'react-powerplug/State.js' than with import {State} from 'react-powerplug'.

Thoughts?

I also know this library is intended more for prototyping than production use, but I'm realizing that <Toggle> would be the most convenient way to handle the various cases of user-toggled collapsible panes in my application.

TrySound commented 6 years ago

Does webpack supports modules.root field? Rollup does not support it. And won't. There's enough proposals for esm. It's better to use existing popular solutions which work well than inventing new one every week.

And this is not treeshaking import State from 'react-powerplug/State.js'. You may call it cherry picking.

Treeshaking is static analyses which allows to introduce only necessary modules from a set of exports, not files. Cherry-picking doesn't solve the problem. It completely omit technology just because you don't believe in it.

Treeshaking works, even better with flat bundles because you can see problems before they will go in user bundle. I created a tool to test treeshakability.

TrySound commented 6 years ago

And this library is not only for prototyping. It's designed to help building apps with simple functions. Dozens of our components use it in production.

jedwards1211 commented 6 years ago

Well shoot, I'm now having trouble finding concrete documentation of whether Webpack supports modules.root, though I don't think I would have wound up believing it does unless I found that somewhere...I'll check on that and get back to you.

Cherry-picking doesn't solve the problem. It completely omit technology just because you don't believe in it.

In packages that do publish multiple modules, I think the advantage of cherry picking with a Webpack build is that if I import {Foo} from 'bar', even if Webpack does tree shaking, it still includes that small amount of code from bar/index.js that imports and re-exports the default from bar/Foo.js, as well as module wrapper functions for both bar/index.js and bar/Foo.js. Whereas import Foo from 'bar/Foo.js' doesn't include the re-export code or module wrapper function for bar/index.js

Obviously this wouldn't be an issue for importing from a rollup bundle.

TrySound commented 6 years ago

Yep, that's the point of using rollup. It produces clean and readable output which even does not require sourcemaps.

jedwards1211 commented 6 years ago

@TrySound it does still support sourcemaps though if I want to use them right? For instance I would find it handy to be able to navigate to the sourcemapped react-powerplug/src/Toggle.js in the browser.

jedwards1211 commented 6 years ago

Do you know for sure if Webpack code splitting works well on rollup bundles? Please don't take my skepticism as being anti-rollup, I just want to make sure that I can meet all my goals using Webpack to build my app when there are rollup packages in my dependencies.

For instance if my chunk A does import {Toggle} from 'react-powerplug' and my chunk B does import {Interval} from 'react-powerplug', will webpack put only the code for Toggle in chunk A and only the code for Interval in chunk B? I would be worried that Toggle and Interval would both wind up together in a third chunk, but hopefully webpack is smart enough to output two different tree-shakings of the same module...

TrySound commented 6 years ago

Yep, webpack includes both Toggle and Value in both bundles. It's webpack bug.

jedwards1211 commented 6 years ago

Looks like it, I was experimenting with their optimization settings like mergeDuplicateChunks: false but haven't found anything that affected the output. I'll file an issue in webpack

jedwards1211 commented 6 years ago

It could definitely be because webpack decided the amount of code was so small that it wasn't worth splitting into separate chunks, but I need to get a definitive answer from the webpack team.

jedwards1211 commented 6 years ago

https://github.com/webpack/webpack/issues/7780

jedwards1211 commented 6 years ago

Actually which version of we pack are you using?

For me it included both Toggle and Interval in one chunk, not both chunks. (But as I mentioned, what I really want is for it to include Toggle in one chunk and Interval in the other.)

TrySound commented 6 years ago

In the latest version I get both toggle and interval included in both chunks. That I meant as webpack bug.

TrySound commented 6 years ago

One chunk have both exports and another one too.

jedwards1211 commented 6 years ago

Do you mind sharing your test code?

TrySound commented 6 years ago
// src/index.js
import('./chunk1.js')
import('./chunk2.js')

// src/chunk1.js
import { Toggle } from 'react-powerplug';
console.log(Toggle)

// src/chunk2.js
import { Value } from 'react-powerplug';
console.log(Value)

// webpack.config.js
module.exports = {
  mode: 'production',
  externals: {
    'react': 'React'
  }
}
jedwards1211 commented 6 years ago

I see. So far I haven't been able to tweak the settings to get it to split the react-powerplug deps, but maybe I'll figure it out.

jedwards1211 commented 6 years ago

What are your thoughts on releasing source maps for the rollup bundles published with a package? I know that the code in the rollup bundle is easy to read, but I don't like the thought of having to do a bunch of Ctrl-F in the debugger to locate the specific code in a dependency that I'm looking for, versus being able to jump to the source module in a dependency with Ctrl-P.