Turn elements/index into an index

[kurkle]

I think we only need the src/index.js to be a monolith.

[benmccann]

I haven't tested it, but from reading the docs it sounds like webpack will do a better job of tree shaking at least in some scenarios depending on the options the user has set if we leave the import statement as import Rectangle from '../elements/element.rectangle';. It sounds like it has different modes where it either looks at just import/export or sometimes tries to figure out if code has side-effects although that's tougher

https://webpack.js.org/guides/tree-shaking/

[kurkle]

I fail to find such information than webpack's guide. Most of the examples import non default exports there.

[benmccann]

Importing non-default export is fine. I'm more worried about transitively importing things that aren't used. I.e. the existence of elements/index. Webpack does tree shaking at a file level by default and not a method level, so this stops the other elements from just falling out

I'm a little less worried about elements/index if nothing refers to it though except the entry point and we make a new entry point for tree shaking which doesn't use it at all

[kurkle]

This should make it possible to have export * from ./elements in that new entry point.

[benmccann]

Should we close this one if we're going with #7149? I'm wondering what our overall plan looks like with tree shaking. #7149 would make it harder to set sideEffects: false, which I think is okay, but then in that case I think it's better to just import what's needed instead of an index file that in turn imports everything else

[kurkle]

I intentionally changed the index file to directly re-export everything instead of bundling them into an object and exporting that. I think most if not all bundlers can handle that.

Also if you look #7149 more carefully, I did the same there. Only thing missing (for scales) is the index.esm.js in /src that re-exports everything we want to export form the .esm.js public package.

[benmccann]

I just tested this in a WebPack project and it didn't remove it

[kurkle]

I just tested this in a WebPack project and it didn't remove it

Thanks for testing. I could not know what you tested and how and what it did not remove, so I tested myself.

The test repo is here: https://github.com/kurkle/webpack-tree-shaking-tests
I included the dist folder so you don't have to build it to see.

Outcome:
In production mode, WebPack (actually Terser) shakes the unused Point element in all the 3 cases I tested.

[benmccann]

Ah, it seems like the difference is I just used WebPack directly whereas you built with Rollup first and then WebPack. I guess it depends how we set things up for tree shaking

Let's say that we build things with Rollup first. How does that work together with the Scale self-registration? I think it'd register all the scales, which would stop them from being shaken out.

[kurkle]

Let's say that we build things with Rollup first. How does that work together with the Scale self-registration? I think it'd register all the scales, which would stop them from being shaken out.

The "self registration" as done in #7149 registers the scale when imported. So it will not make all scales register, unless all are imported. Other options are internal registration of all scales (disables tree shaking) and some kind of post import registration. And of course the "no registration", where the scales would be defined with instance or constructor instead of the current name.

[benmccann]

The "self registration" as done in #7149 registers the scale when imported. So it will not make all scales register, unless all are imported.

The entry point compiled by rollup may have to import everything, but it depends how we set things up.

I think there's a few different ways we can set things up and it's hard for me to figure out what we want to do here and in #7149 without the full picture of where we're going. Maybe we should have a ticket just to discuss the overall higher-level strategy or discuss on Slack?

I think there's two main options, which I tried to outline in #7009 (comment). I think this PR is helpful for option 1 and self-registration is helpful for option 2. I don't know that it makes sense to do both. We originally decided in #6979 that option 2 wasn't feasible because of the CSS plugin, but now that it's been removed I think it should be feasible and would be my preference.

[benmccann]

This should be fine assuming we create a new entry point for tree shaking, get rid of auto-registration, and set sideEffects: false