shoelace-style / shoelace

A collection of professionally designed, every day UI components built on Web standards. SHOELACE IS BECOMING WEB AWESOME 👇👇👇
https://webawesome.com
MIT License
12.86k stars 821 forks source link

Distribute bare module specifiers instead of bundled Lit dependency #559

Closed aomarks closed 1 year ago

aomarks commented 3 years ago

What issue are you having?

Shoelace is distributed with its own copy of Lit, instead of referencing it as a dependency, so if there is any other code on the page that also uses Lit, there will be two copies. There is no way to de-duplicate the dependency currently, causing an unnecessary inflation of JavaScript shipped to the browser.

Example: https://unpkg.com/browse/@shoelace-style/shoelace@2.0.0-beta.53/dist/components/button/button.js imports https://unpkg.com/browse/@shoelace-style/shoelace@2.0.0-beta.53/dist/chunks/chunk.X3WLUTHF.js which is a copy of Lit.

Describe the solution you'd like

Distribute bare module specifiers instead of bundling a copy of Lit. This way all code that needs Lit can reference the same copy of it.

This is the solution used by most other web component libraries I've encountered so far:

MWC: https://unpkg.com/browse/@material/mwc-button@0.25.1/mwc-button-base.js Lion: https://unpkg.com/browse/@lion/core@0.18.3/index.js Spectrum: https://unpkg.com/browse/@spectrum-web-components/base@0.4.5/src/Base.js Vaadin: https://unpkg.com/browse/@vaadin/vaadin-button@21.0.2/src/vaadin-button.js

See also this Twitter thread where this was discussed recently: https://twitter.com/straversi1/status/1425275394853195777

cc @justinfagnani @straversi

claviska commented 3 years ago

This is on my list, for sure. The immediate problem is the custom build, which needs to be reworked to use @web/dev-server or similar. Currently, the "dev build" is the same as the prod build and it gets fully regenerated when files are changed. Meanwhile, the dev server is powered by BrowserSync.

I (briefly) tried fixing this a few weeks ago, but I wasn't able to get BrowserSync to follow bare module specifiers. If that could be solved, we can add lit to esbuild's external config and I'm pretty sure that would do the trick.

claviska commented 3 years ago

Thanks to a suggestion by @ParamagicDev, I'm pretty sure this is resolved in the build branch I just pushed. The important change is here:

https://github.com/shoelace-style/shoelace/blob/build/scripts/build.js#L56

In short, the dev build stays the same and the prod build sets all deps as external so esbuild doesn't bundle Lit, Popper, etc. I could use more eyes on this to confirm, though.

@justinfagnani we talked about how this will work with popular CDNs such as jsDelivr and UNPKG recently. Do I need to do anything special in package.json, or will CDNs know how to handle bare module specifiers out of the box?

aomarks commented 3 years ago

Do I need to do anything special in package.json, or will CDNs know how to handle bare module specifiers out of the box?

Looks like you have lit and @popperjs/core in your package.json dependencies already, so that should be all you need.

Maybe do a pre-release to NPM? Happy to try it out!

KonnorRogers commented 3 years ago

@claviska I have a package that marks morphdom as external using Rollup and when I push it to Skypack / Unpkg, it appears to work out of the box with no additional config. I don't think you should have any issues.

claviska commented 3 years ago

Welp, beta.54 broke the CDN.

<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.54/dist/themes/light.css">
<script type="module" src="https://cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.54/dist/shoelace.js"></script>

<sl-button type="primary">Click me</sl-button>
Uncaught TypeError: Failed to resolve module specifier "lit". Relative references must start with either "/", "./", or "../".

It does work through esm.run, but that's "beta" and if it doesn't work in jsDelivr it probably won't work with others. I've reverted to bundling in beta.55 for the time being.

The simplicity of the CDN is vital to this project. What other options do we have? Do we need to wait for import maps to be better supported?

justinfagnani commented 3 years ago

Can you publish a separately bundled version? I wouldn't want to depend on a library that bundled its own copy of Lit. Once you do bundle, that can't be unbundled.

KonnorRogers commented 3 years ago

@claviska I would create a seperate dist/shoelace.min.js that includes the fully bundled lit. So you would have 2 final bundles:

dist/shoelace.min.js
dist/shoelace.js

dist/shoelace.js would mark Lit as external dist/shoelace.min.js would bundle up Lit

For people grabbing from a CDN they can go here:

"https://cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.54/dist/shoelace.min.js"

For people grabbing for local use off npm, they wont have to change anything.

jaredcwhite commented 3 years ago

@claviska I would create a seperate dist/shoelace.min.js that includes the fully bundled lit. So you would have 2 final bundles:

That's really what I would prefer as well. I think it's cool you can import bundled shoelace off a CDN and go to town, but I never do that in any substantial project of mine (and I'm often using Lit as well so it'd be a waste to use my own Lit + Shoelance's Lit on the same page).

justinfagnani commented 3 years ago

I would actually prefer to see shoelace published to npm completely unbundled. I can bundle components myself much better than shoelace can since I know my entrypoints. I see that individual components import different chunks, but that should be unnecessary if the app is bundling.

Could there be a separate @shoelace-style/shoelace-dist package that has bundles?

claviska commented 3 years ago

I’ll play around with more ideas on Monday. After thinking it over, I’m OK using esm.run as the “official” CDN. That means the npm version will be unbundled.

The only other question is should the docs continue to use a local dist? They need to for dev and next since there’s no corresponding package, but maybe prod can use the CDN.

We’ll get it there for the next release — just need to work out that last detail.

claviska commented 3 years ago

One thing that really sucks about this is having to create two dists for bundled and unbundled. That means assets, metadata, etc. all have to get duplicated. The redundancy feels sloppy.

I'm going to need more time for this one.

claviska commented 3 years ago

To clarify, the unbundled version from beta.54 does work with CDNs that support bare module specifiers.

<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.54/dist/themes/light.css">
<script type="module" src="https://cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.54/dist/shoelace.js/+esm"></script>

<sl-button type="primary">Click me</sl-button>

Ideally, I only want to ship the unbundled version on npm. The current issue is:

It's easy enough to produce those builds, but assets need to be generated multiple times now. That's the part I'm trying to work out.

claviska commented 3 years ago

As of beta.56, all dependencies are unbundled except for color which is used by the color picker. Unfortunately, it doesn't ship an ESM version so I wasn't able to exclude it at this time. Fortunately, you'll only receive that if you use the color picker.

In the meantime, I've offered to work with the author to provide an ESM version: https://github.com/Qix-/color/issues/184

I'm leaving this open until that's also resolved.

aomarks commented 3 years ago

Thank you for the work on this!

claviska commented 3 years ago

Some good news regarding color. https://github.com/Qix-/color/issues/184#issuecomment-942754062

claviska commented 3 years ago

I should have tested this more thoroughly. Unbundling and shipping via CDN is causing problems. While I appreciate the value of this:

More importantly, singleton patterns are breaking because the CDN is bundling modules separately for some reason. Shoelace uses a couple of these, the most import one being to set the base path for assets.

This worked fine in previous versions:

https://codepen.io/claviska/pen/zYdomxP?editors=1000

It's broken now:

https://codepen.io/claviska/pen/vYJyVYb?editors=1000

If you look at the source of each module in the latter example, you'll see that dist/utilities/base-path.js has been bundled by the CDN in both files:

I consider this a jsDelivr bug, as unpkg doesn't exhibit this behavior:

https://codepen.io/claviska/pen/eYEBPYR?editors=1000

However, I'm reluctant to make unpkg the "official CDN" because, historically, jsDelivr has been noticeably faster with less latency compared to unpkg. jsDelivr also offers critical stats that I can't get from another public CDN.

I'm going to file a bug with jsDelivr, but I may need to roll back this change for now.

claviska commented 3 years ago

Bug filed: https://github.com/jsdelivr/jsdelivr/issues/18337

claviska commented 3 years ago

If you look at the source of each module in the latter example, you'll see that dist/utilities/base-path.js has been bundled by the CDN in both files

Update: I guess this isn't a bug, it's a feature.

justinfagnani commented 3 years ago

That's really, really unfortunate. They're breaking module semantics.

The problem here is that if you twist your library to work better on buggy module CDNs you might force module duplication on those installing via npm (if you bundle dependencies). I would consider local installs the more common and important constituency here.

I would at the very least keep bare specifiers for imports to external modules like lit so that they're not duplicated on local installs. You could still bundle everything into one file so there's duplication of Shoelace - but only when the user is using one version of Shoelace.

That last point is important because this problem is simply not solvable with one package opting into bundling. If a third-party component is using Shoelace and a page loads both Shoelace and that component, the page could double-load all of Shoelace. Pre-bundling at the package level only kicks the problematic can down the road. Bundling should only be done at the app/page level.

straversi commented 2 years ago

👋 Hi, I super appreciate the time spent looking into this, and definitely see problems that are blocking this.

I'm starting a new Lit project today, and as sad as it makes me (I love Shoelace), I can't justify shipping two copies of Lit.

So at least for me, this issue is blocking Shoelace usage at all for me going forward as long as I'm using Lit.

Again, LOVE shoelace, just wanted to add 2 cents of real-world input!

MartinKolarik commented 2 years ago

I would at the very least keep bare specifiers for imports to external modules like lit so that they're not duplicated on local installs.

This is the most versatile solution (and used by most packages). It should work with the CDNs and allow sharing dependencies at the same time. We're still considering adding a raw/unbundled mode to jsDelivr but even if we do, bundling your own code and keeping other dependencies external might give you better performance.

claviska commented 2 years ago

I haven't been able to come up with a best of both worlds solution, short of shipping two completely different packages.

From this thread, @justinfagnani writes:

This completely breaks standard module semantics. One huge reason to even make a separate module is so you can have common state like caches. Packages I maintain, like lit-html and LitElement, use things like per-module caches, class statics (that are effectively per-module). Not sharing the caches will cause a lot of duplicated work that'll slow down a page.

This concerns me because, if I ship bare specifiers for deps and go back to using the /+esm routes on jsDelivr, it's not clear what singletons in those deps will be borked from module duplication or when it might happen. The result could be benign or it could turn into a performance problem, and it could vary from version to version. Bundling Lit seems to be the only reliable way to get it to work without that risk, as far as I understand. It's also not clear how to avoid the original problem I had with /+esm routes for internal modules.

I really want to solve this because I know it's not ideal in its current state, but to solve it today using jsDelivr I'd have to ship two separate packages (which isn't ideal for devs and will definitely confuse users) or use an alternate CDN. In my experience, jsDelivr has been by far the most reliable.

How do folks feel about unpkg? I previously used it for Shoelace but it was noticeably slower, although it does seem to handle bare specifiers correctly. I think it was removed, but I also recall a pretty blatant warning that it wasn't intended for production. Not sure if that changed at some point. I'm pretty sure there are still zero analytics though. 😢

Skypack is sometimes very slow initially but it offers pinned URLs which might help with that. I think they have to be generated after publishing though, so not sure how I could feed them back to the docs without reworking the build. They don't seem to have much for stats, but at least they show weekly downloads. 🤷🏻‍♂️

Is there any other option?

MartinKolarik commented 2 years ago

Note that AFAIK, Skypack uses more or less the same approach as jsDelivr - 1 bundle per package. The unpkg's not bundling approach is the only way to ensure everything works 100% correctly but it doesn't scale. One package can have hundreds of files. When that turns into hundreds of requests, even with HTTP2/3 and preloading, it's quite slow (we have a benchmarking tool at https://www.jsdelivr.com/esm that you can use to compare this with any npm module). And of course, a typical page loads more than just one package. So there's a big tradeoff between correctness and real-world usability.

straversi commented 2 years ago

Would it be weird to ship the unbundled version in the same package, just in a separate directory?

If I discovered I could import from shoelace-style/shoelace/unbundled, it wouldn't bother me personally from a dev-x perspective.

justinfagnani commented 2 years ago

The problem there is the hazard of ever worse duplication. If an ecosystem of 3rd party components that compose Shoelace grows up, some could import the bundled version, and others the unbundled. You could get errors on registering custom elements and code duplication of more than just Lit.

Of course a 3rd party extension would highlight the problems with bundling anyway. If they bundled Shoelace like Shoelace bundles Lit, then you'd have all that some duplication anyway - which points to bundling not generalizing.

Ultimately this seems like a CDN problem. The correct way to reference dependencies is via named imports. CDNs should work with named imports. Far from solving duplication, bundling Lit is going to cause duplication if anything else uses Lit. It only works if Shoelace is the only thing loaded.

If it's necessary for some reason though like a CDN bug, maybe export conditions are a way around this as they can express a cross-cutting concern that applies to all imports. You could document that enabling an export condition, like unbundled would choose a different build. It'd be unfortunate to have to opt-out of bundling in an npm package, but at least it'd be possible.

claviska commented 2 years ago

And of course, a typical page loads more than just one package. So there's a big tradeoff between correctness and real-world usability.

Understood, and while Shoelace has a small number of dependencies, I understand that many projects do not so this would result in a TON of undesirable HTTP requests.

This isn't an easy problem to solve, but maybe a custom property in package.json could allow authors choose to how jsDelivr bundles their modules. I'd be OK with an "all or nothing" option or something more granular, e.g. "don't duplicate these selected modules..." even if it means more work for me as a library author.

Would it be weird to ship the unbundled version in the same package, just in a separate directory?

The problem there is the hazard of ever worse duplication. If an ecosystem of 3rd party components that compose Shoelace grows up, some could import the bundled version, and others the unbundled.

I thought about this, and the build is configurable to allow for it. You can try it by running:

node scripts/build.js --types --dir \"dist-unbundled\"

That will generate an unbundled build in dist-unbundled, but to Justin's point, a different version of the library could now be referenced from the same package. We could somewhat alleviate that by shipping a separate package, e.g. @shoelace-style/shoelace-unbundled but there's still a risk of multiple projects using Shoelace and not being aware of which one despite being on the same "version."

Flipping this idea around (and considering the known issues), would it be better to unbundle @shoelace-style/shoelace to make it "pure" for npm/bundlers/unpkg and ship a static, CDN-capable version in a new package called @shoelace-style/shoelace-cdn?

The CDN package would be for folks who want the convenience of a traditional CDN (e.g. no build step, just copy and paste). The chance for duplication is still possible, but it falls on the user to be aware of other things they're using that might rely on Shoelace. I doubt most CDN users will have this problem, as they seem to be copying and pasting and using the lib as-is in their webpages.

I'm betting on this being more a problem for npm users, specifically other Lit users that are building web components with Shoelace as a dependency.

MartinKolarik commented 2 years ago

This isn't an easy problem to solve, but maybe a custom property in package.json could allow authors choose to how jsDelivr bundles their modules. I'd be OK with an "all or nothing" option or something more granular, e.g. "don't duplicate these selected modules..." even if it means more work for me as a library author.

I think this is possible to some extent even now because we apply browser field mappings, which can change internal paths to external modules, and also the new imports field (https://nodejs.org/dist/latest-v17.x/docs/api/packages.html#subpath-imports), which can do the same (this one is newer and better standardized but requires writing the actual import in your code in a little different way).

There are two limitations I see right now:

  1. The overrides are always configured per file, so if you needed to exclude too many files, the config could get quite long.
  2. I suppose you'd want this to apply only when bundling at jsDelivr. The fields might currently be picked up by webpack for npm users too - but that's something we might be able to solve. The imports field specifically supports "conditions" and we could add one that's used only by us. Alternatively, the browser field syntax could be also supported by our own jsdelivr field, which currently only supports strings.
justinfagnani commented 2 years ago

One big downside to bundling dependencies that Shoelace has now is that Lit's export conditions aren't re-exported by Shoelace. Shoelace bundles only Lit's production builds and leaves out the development build and new Node build.

For development this means that authors of subclasses of Shoelace components won't get extra runtime checks and nicer warnings. That might not be a supported use case, but it's there.

For Node is means that Shoelace components won't automatically pick up the Lit node builds that don't require a DOM shim to load in Node. This will potentially make Shoelace harder to use in 11ty, Next, etc apps than other Lit components.

rstoneIDBS commented 1 year ago

Any progress on this issue? I got here as a result of the "Multiple versions of Lit" message appearing in my browser console. I'm using Shoelace via npm but also have my own Lit components so would really like to avoid this duplication issue :( Shipping two versions (bundled for CDNs and unbundled for npm) would seem to be the best solution for now given the current state of play, at the moment your npm users are paying the price of CDN simplicity :(

mpharoah commented 1 year ago

I am also running into this. Ultimately, Lit is small enough, that its not a big deal, but I would still love to get rid of this ugliness from the weird way Shoelace does its bundling.

justinfagnani commented 1 year ago

Remember that it's not just the duplication that's the problem. Lit is using Node package exports to provide different builds for dev, prod, and Node. Shoelace bundling Lit means that it has to choose a build (presumably prod) and developers can't get the other builds by change the import conditions.

Not getting the dev build can make things harder on devs extending ShoelaceElement, or make it difficult to identify a bug in Shoelace. Not getting the Node build means that for SSR use cases, Shoelace users need to load a separate DOM shim since they can't use the Node build which doesn't call DOM APIs on startup (see https://github.com/lit/lit/issues/3770).

claviska commented 1 year ago

@justinfagnani do you have time this week or next week to outline a plan for this? It seems to be much more than a CDN and an npm version, and I want to make sure we tackle as many of these cases as possible. Feel free to throw something on my calendar, if that's easier.