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

Better tree shaking usage #525

Closed
songhn233 opened this issue Sep 15, 2021 · 5 comments
Closed

Better tree shaking usage #525

songhn233 opened this issue Sep 15, 2021 · 5 comments
Assignees
Labels
feature Feature requests.

Comments

@songhn233
Copy link

Is your feature request related to a problem? Please describe.

Reference to #180, Most bundle tools will not tree shaking correctly due to class extends may create side effect.

And at the end of the issue, the solution is given

import { SlButton } from '@shoelace-style/shoelace';
SlButton.register();

However, the way it is introduced has changed and I have not found any discussion of it.

Since the decorator @customElement() is now used, the import will have the side effect that the component will be automatically defined.

So is it possible to provide the class directly and then have the user register it manually? This way class annotations /* @__PURE__ */, esbuild or rollup will be handled correctly for bundle, and side effects within the specified scope of tools like webpack will work. There are other benefits, such as modifying the tag names of registered components.

Now if we directly import @shoelace-style/shoelace it will result in an oversized bundle size. The current cherry pick method leads to a lot of duplicate code, as in the documentation

import '@shoelace-style/shoelace/dist/themes/light.css';
import '@shoelace-style/shoelace/dist/components/button/button.js';
import '@shoelace-style/shoelace/dist/components/icon/icon.js';
import '@shoelace-style/shoelace/dist/components/input/input.js';
import '@shoelace-style/shoelace/dist/components/rating/rating.js';

I think the original scheme of importing components and then registering them would be more appropriate, and also conducive to tree shaking. This behavior is also similar to using components in other frameworks, such as Vue.

If cherry picking cannot be avoided, it is necessary to provide a solution similar to babel-plugin-import.

@songhn233 songhn233 added the feature Feature requests. label Sep 15, 2021
@claviska
Copy link
Member

So is it possible to provide the class directly and then have the user register it manually?

Not when using Lit's @customElement decorator. Auto-registration is the recommended approach for both Lit and FAST Element and I'd prefer to stick to that recommendation.

It's worth mentioning that this issue is described in the docs under Cherry Picking, and cherry picking remains the recommended approach for minimal file size.

As import maps become better supported by browsers, bundlers will follow suit and that will be the ideal solution.

If cherry picking cannot be avoided, it is necessary to provide a solution similar to babel-plugin-import.

If someone were to release a Babel plugin, I'd gladly link to it from the installation page.

@mvromer
Copy link

mvromer commented Nov 18, 2021

Not directly related to the tree shaking aspect but more on the auto-registering aspect, I've run into one scenario where component auto-registration could cause some problems. In my use case, we're exploring the possibility of designing our frontend web app using micro frontends that are developed by independent teams. I'm pushing hard internally for us to build libraries of common components based on Web Components (specifically using lit) that each of the teams can use when building out their micro frontends regardless which frontend frameworks are used to build them. I really liked the design of Shoelace and the ability it gave me to customize it to fit our needs, so just as the Overview states, we've started using Shoelace as a foundational piece in our component libraries.

One challenge I'm starting to see is that unless we use something like import maps or perhaps module federation (I'm pretty ignorant on the latter), if two teams independently bundle a component that depends on a Shoelace component, a browser runtime exception will occur because the Shoelace component gets registered twice. To mitigate that, I wanted to explore using Open WC's Scoped Elements mixin, but as it states down in its limitations, imported components shouldn't be self-registering. Ideally this wouldn't be a problem if all teams stayed on the same version of the Web Component package (again relying on import maps/module federation), but that's not going to be feasible 100% of the time as the Open WC docs page illustrates here.

I know lit advocates for auto-registering, but I'd be curious to know if that guidance still holds if/when scoped element registries lands in browsers. In the meantime, would you be open to reconsidering ways of exporting Shoelace's components that support both the auto and the manual registering case? If there's a design you'd be comfortable with, I'm happy to take a stab at a PR. This would definitely make it more feasible for someone to use Shoelace in cases where they want/need to also scope the custom element registration either via polyfill today or native browser support sometime in the future. Or if there is an alternate way to support this use case with Shoelace, I'd be happy to explore that too on my end.

@claviska
Copy link
Member

Admittedly, I was torn between auto-registration (which causes side effects) and manual-registration early on. The latter shifts a significant burden to the user by doubling the lines of code required to import components (in both vanilla and when using React wrappers) and, depending on how it's configured, requires them to register the correct tag names.

At the moment, I'm still intent on auto-registration since that happens automatically in both Lit and FAST when you use the @customElement decorators. This means I don't have to reinvent my own decorator, the caveat being you can't consume everything from the root [for now].

@claviska
Copy link
Member

There are other benefits, such as modifying the tag names of registered components.

This isn't currently supported in Shoelace. Various internal things such as DOM traversal rely on the current tag names (e.g. when a menu looks for slotted menu items, slotted styles that target the tag name, etc.).

I'd like to make the prefix and perhaps even the full tag name customizable, but it wasn't originally planned this way and it will probably have to wait for 3.0.

@claviska
Copy link
Member

I'm closing this as it's getting stale and there's no clear path forward. Currently, you can load the entire library through shoelace.js or cherry pick individual components from their respective path. I'm following Lit's recommendation to auto-register for the time being.

I believe the main complaint herein will be solved with import maps, so please subscribe to #517 if you're interested in that as a potential solution.

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

No branches or pull requests

3 participants