Closed subdavis closed 3 years ago
Looks like CI is failing... is this ready for review?
Only if you're interested in making these changes. I'll clean it up and write docs, but only if others think this is useful.
It seems like a good quality of life enhancement for downstreams as well as developers of GWC itself. Plus, it seems inevitable that everything we want to carry forward long term will be moving to typescript eventually.
I accept your arguments as written (to the extent that I think you should distill your PR description down into a general purpose blog post or RFC or other written artifact).
One question I have about registerComponents()
--the way I've seen that done in other places (including Vuetify) is through Vue.use()
to register a plugin, presumably along with its components. Why aren't we doing that?
Why aren't we doing that?
Tree shaking. Vuetify had to write an entire webpack loader to permit both a-la-carte bundling and global component registration, and it still makes my brain hurt sometimes, such as in vue-media-annotator where you have to delay component resultion to downstream, but vuetify-loader only happens in SFCs, so you're stuck with global registration when using that library anyway.
Let's look at the extreme example. Right now, this library doesn't tree shake at all, really. You can prove this by going into demo/main.js
and commenting out the App.vue
import, then running yarn build:demo
:
File Size Gzipped
_site/js/chunk-vendors.e910351f.js 572.87 KiB 189.90 KiB
_site/js/app.8465e7c4.js 90.07 KiB 23.01 KiB
_site/css/chunk-vendors.f97f547f.css 717.11 KiB 95.58 KiB
_site/css/app.1198ec8b.css 8.60 KiB 1.71 KiB
Compare this with a yarn build:demo
of current master:
_site/js/chunk-vendors.05154e1b.js 581.93 KiB 192.05 KiB
_site/js/app.0da33550.js 108.03 KiB 27.12 KiB
_site/css/chunk-vendors.633d9cb7.css 722.11 KiB 96.31 KiB
_site/css/app.92bfade8.css 8.76 KiB 1.80 KiB
This app, with no vue components being used at all, is only barely smaller than if it used a ton of stuff from gwc and vuetify.
Perform the same test after this change. Now, if you comment out the app import, you get a bundle that looks like this:
File Size Gzipped
_site/js/chunk-vendors.430989a1.js 211.04 KiB 74.79 KiB
_site/js/app.ed677a8b.js 9.85 KiB 3.64 KiB
_site/css/chunk-vendors.1016e3fd.css 289.82 KiB 32.42 KiB
The crucial part is app.hash.js
. Our part was reduced by a factor of 10. As a result, vuetify's part was reduced becasue of vuetify-loader. Something else cool that happened was that app.hash.js
ate the app.hash.css
rather than rolling all the component styles into a css bundle on their own. I don't totally understand why.
To complete the circle, if you stuck registerComponents
into the plugin install function, it would include everything again, and you'd be back to where you started with no tree shaking.
Anayway, this confirms my initial assertion above about how tree shaking wasn't actually happening on this code.
Why aren't we doing that?
Tree shaking. Vuetify had to write an entire webpack loader to permit both a-la-carte bundling and global component registration, and it still makes my brain hurt sometimes, such as in vue-media-annotator where you have to delay component resultion to downstream, but vuetify-loader only happens in SFCs, so you're stuck with global registration when using that library anyway.
Example
Let's look at the extreme example. Right now, this library doesn't tree shake at all, really. You can prove this by going into
demo/main.js
and commenting out theApp.vue
import, then runningyarn build:demo
:File Size Gzipped _site/js/chunk-vendors.e910351f.js 572.87 KiB 189.90 KiB _site/js/app.8465e7c4.js 90.07 KiB 23.01 KiB _site/css/chunk-vendors.f97f547f.css 717.11 KiB 95.58 KiB _site/css/app.1198ec8b.css 8.60 KiB 1.71 KiB
Compare this with a
yarn build:demo
of current master:_site/js/chunk-vendors.05154e1b.js 581.93 KiB 192.05 KiB _site/js/app.0da33550.js 108.03 KiB 27.12 KiB _site/css/chunk-vendors.633d9cb7.css 722.11 KiB 96.31 KiB _site/css/app.92bfade8.css 8.76 KiB 1.80 KiB
This app, with no vue components being used at all, is only barely smaller than if it used a ton of stuff from gwc and vuetify.
Perform the same test after this change. Now, if you comment out the app import, you get a bundle that looks like this:
File Size Gzipped _site/js/chunk-vendors.430989a1.js 211.04 KiB 74.79 KiB _site/js/app.ed677a8b.js 9.85 KiB 3.64 KiB _site/css/chunk-vendors.1016e3fd.css 289.82 KiB 32.42 KiB
The crucial part is
app.hash.js
. Our part was reduced by a factor of 10. As a result, vuetify's part was reduced becasue of vuetify-loader. Something else cool that happened was thatapp.hash.js
ate theapp.hash.css
rather than rolling all the component styles into a css bundle on their own. I don't totally understand why.Edit
To complete the circle, if you stuck
registerComponents
into the plugin install function, it would include everything again, and you'd be back to where you started with no tree shaking.Anayway, this confirms my initial assertion above about how tree shaking wasn't actually happening on this code.
A 10x bundle size savings is impressive. Thanks for the technical walkthrough.
@zachmullen @BryonLewis @brianhelba @waxlamp @AlmightyYakob bump
Feel free to remove yourselves from review request.
This represents a somewhat significant change in the API surface of GWC.
Type Definitions
Typedefs will greatly improve the DX for projects already using typescript, and will provide more robust editor hints/autocomplete for projects that don't. This is the least controversial change.
More explicit tree shaking support
I'm still looking into whether or not this actually helps, but it certainly makes my brain hurt less when I think about how library code gets included in my project.
Adds
sideEffects
package.json argument, which explicitly marks all our js/vue files as side-effect free, which webpack appreciates. https://webpack.js.org/guides/tree-shaking/Components all renamed to Girder* to match documentation
All of the changes I made here are directly inspired by the Vuetify project architecture.
In that spirit, I've renamed the exported public symbol for every component to
GirderWhatever
. The file names haven't changed, but all components exported from the package index are prefixed. See below.This was done because renaming every import is just kinda annoying, but it's a best practice, so there's really no reason this library can't do it for you. If you still want to rename the import, nothing's stopping you from doing so.
This makes the exported names match the docs automatically.
Full export reorg
This library has always bothered me a bit. I don't like that you have to go spelunking through the code to see what's actually included in this library. Most libraries don't do this, they just export all their symbols at the package level. We certainly don't have a complicated lib like d3, so I don't think we have a valid excuse to have such complicated imports.
New style:
export * from './submodule';
Also, previously, arbitrary parts of this library simply weren't exported, so you could get to them by path, but not with the umd bundle. I've gone through and zealously exported every part of the library that I've ever used in a downstream app. The sub-folders under
components/
now all look the same. My rationale is that we're all smart people who use this thing often, so more control is better.Along with the tree shaking support above, I believe (perhaps mistakenly) that bubbling exports up to the package level will allow webpack to better optimize. For example, previously,
import { components } from '@girder/components/src
would force inclusion of all components regardless of how many you used.Now, you can just
import { GirderFileManager } from '@girder/components';
and only that component (and the side-effects along its import tree) will be included.We shouldn't make developers jump through mental hoops by having many different ways to accomplish the same thing with possibly hidden consequences.
For the 80% case,
import { Thing } from '@girder/components
will be the easiest, most efficient way to use something from this library. If you do your imports like this, you won't run into side effect problems like we all have in the past.Note: I changed the
package.json
module
andmain
pointers so that@girder/components/src
becomes@girder/components
because that's a pretty unusual thing to do.registerComponents optional method
Some of the first feedback I ever got on this library was confusion about why the developer had to import each component. This adds a simple helper function to register the main components globally, so you can do this:
This is a partial contradiction to my argument for strict persrciptiveness above. For this reason, I'm not attached to having this function, but I think it's kinda nice.
Removed default vuetify instance
Edit: reverted this, since tests and demo app use it and it makes the docs in README nicer
Because I've personally never seen it used
new Vuetify(config)
is not really a feature this library needs to provide.Haven't made README changes yet, pending discussion
I've tagged most GWC stakeholders for comment. If you aren't interested in this change, feel free to ignore.