Create non-auto-registering routes

[KonnorRogers]

Alright. This is a massive PR addressing many things.

Fixes #1437 #1431 #1375 #874 #705

Summary of changes:

• Added JsDoc comments to React Wrappers for better documentation when hovering a component.
• Added displayName to React Wrappers for better debugging.
• Split auto-defining routes and non-auto-defining routes for Components.
• Fixed React Treeshaking by introducing sideEffects key in package.json.
• Fixed a bug in <sl-tree> where it was auto-defining <sl-tree-item>.
• Added a console warning if you attempt to register the same Shoelace component twice.
• Added a shoelace.js and shoelace-autoloader.js to exportmaps.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
shoelace	✅ Ready (Inspect)	Visit Preview	Jul 24, 2023 3:37pm

[claviska]

Looking great! Left some initial suggestions.

[michaelwarren1106]

this PR is awesome! I'm definitely waiting on this to merge so that I can install and re-register a few shoelace comps like sl-popup.

I have some question about the self-definition feature in here:

1. what's the reason for this approach over scoped elements?
2. if someone like me is gonna extend and re-register and needs to add scoped-elements would the approach here conflict?

Looking at the code, it seems like all an extending class would need to do is reset definitions to undefined and the ShoelaceElement constructor code wont run?

[KonnorRogers]

I have some question about the self-definition feature in here:

1. what's the reason for this approach over scoped elements?

Honestly, I keep going back and forth on this. I want to be able to use the open-wc Lit version for now since it does NOT fallback to global registration. I guess the only thing missing would be versioned registration error messages. I'm mostly just scared of pulling in the package and the entire Scoped Elements proposal changes.

2. if someone like me is gonna extend and re-register and needs to add scoped-elements would the approach here conflict?

It shouldn't. You'll get console warnings if the global element already exist, but you should still be able to scope if you need to.

Looking at the code, it seems like all an extending class would need to do is reset definitions to undefined and the ShoelaceElement constructor code wont run?

Correct.

[michaelwarren1106]

Honestly, I keep going back and forth on this. I want to be able to use the open-wc Lit version for now since it does NOT fallback to global registration. I guess the only thing missing would be versioned registration error messages. I'm mostly just scared of pulling in the package and the entire Scoped Elements proposal changes.

I get it. We use scoped elements everywhere and its definitely a pain. It makes the React Wrappers less worth it because it requires those wrappers be imported client-side and i think that folks using react wrappers (especially in nextjs) might just assume that the react wrappers work server-side because they have react structure/syntax. Also the polyfill is a bit of a pain because its an application responsibility to make sure it gets imported before any WCs and some existing apps here are super complex and nested apps in weird monorepos and that can be hard to get all the imports into the root index so that the polyfill comes first.

That said, I think i'd still keep with scoped elements because scoping enables multiple versions of the same component to exist on the same page at the same time even if those components are internal to some other component. If alerts uses icon, then the version of icon in alert doesnt have to be the same as the version of icon in the app using alert. Thats a major benefit imo.

Let me know if you want to talk shop about scoped elements and the approach. I use the polyfill and the ScopedElementsMixin from open-wc. I've thought about switching to the Lit one, but they arent that much different

[KonnorRogers]

I get it. We use scoped elements everywhere and its definitely a pain. It makes the React Wrappers less worth it because it requires those wrappers be imported client-side and i think that folks using react wrappers (especially in nextjs) might just assume that the react wrappers work server-side because they have react structure/syntax. Also the polyfill is a bit of a pain because its an application responsibility to make sure it gets imported before any WCs and some existing apps here are super complex and nested apps in weird monorepos and that can be hard to get all the imports into the root index so that the polyfill comes first.

These are the exact pains I don't know if we're ready to dump onto consumers.

That said, I think i'd still keep with scoped elements because scoping enables multiple versions of the same component to exist on the same page at the same time even if those components are internal to some other component. If alerts uses icon, then the version of icon in alert doesnt have to be the same as the version of icon in the app using alert. Thats a major benefit imo.

Yup. I believe this is a major benefit. However, I'm not sure we've even worked out what happens when Shoelace is loaded across different versions on the same page. Particularly around styles which currently register to :root, .sl-theme-light, sl-theme-dark and would overwrite each other or have missing styles when loading multiple version. This is a use-case we want to support as we realize it could allow users to build their own components as NPM packages using shoelace internally. I think I'm okay with leaving this as an opt-in feature until we can work out some kinks on our end. It's not a no, just a not yet until we can have a better story around scoped elements and make sure the library itself would still work as expected.

EDIT: Perhaps we consider this a step in the direction towards scoped elements, at least we now have the option of not always auto-registering on import.

[KonnorRogers]

So in talking with @claviska it appears we may have another use-case to support for tree-shaking for non-react users without needing to cherry-pick.

This would be a breaking change, so perhaps in the 2.x release cycle it would live at @shoelace-style/shoelace/component.js.

The key use-case being around easy integrations with things like Vue / Svelte, etc

Will report back as we discuss this more!

[michaelwarren1106]

So in talking with @claviska it appears we may have another use-case to support for tree-shaking for non-react users without needing to cherry-pick.

so does that mean that this PR has more work before getting merged? no rush, but I'm waiting on this PR to pull in some shoelace components into my project that I want to re-register :)

[claviska]

Yes, it requires a bit more work. Also, while you will be able to re-register, we still don’t support changing tag names and I’d consider that an unsupported use case for the time being.

[weareoutman]

Hi, thanks for your work, but the sideEffects settings here seems to break the cherry picking usage such as:

import '@shoelace-style/shoelace/dist/components/button/button.js';

Since the dist/components/button/button.js also imports dist/chunks/*.js which are not marked as sideEffects, which will be tree-shaked in production mode.

[KonnorRogers]

Do you have a repo by chance?

What bundler are you using? I tested mainly with Vite, Webpack (via CRA), and Parcel.

In Stackblitz using NPM + Vite, it seems to work.

https://stackblitz.com/edit/vitejs-vite-42iegf?file=main.js,package.json,index.html&terminal=dev

[weareoutman]

I'm using Webpack. But your demo is just ok to reproduce it. Try npm run build && npm run preview. Tree-shaking only happens in production mode by default, so npm run dev is not the case.

[Antony92]

Also the docs for 2.6.0 version suggest to import for bundler like so import '@shoelace-style/shoelace/dist/components/button/button.component.js'; but by doing this you also break the dev environment.

Using the same demo @KonnorRogers provided I've just changed the import for the button.

https://stackblitz.com/edit/vitejs-vite-n3cvck?file=main.js

Not sure if this is related to the problem or it's a docs error. @claviska

[KonnorRogers]

@Antony92 Good call on the new import docs. Its probably pulling from the manifest 🤔

[KonnorRogers]

@Antony92 🤔 where in the docs does it say to use the .component.js import? There's a note about auto-registering, but thats different.