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

Distribute bare module specifiers instead of bundled Lit dependency #559

Closed
aomarks opened this issue Oct 6, 2021 · 32 comments · Fixed by #1369
Closed

Distribute bare module specifiers instead of bundled Lit dependency #559

aomarks opened this issue Oct 6, 2021 · 32 comments · Fixed by #1369
Assignees
Labels
feature Feature requests.

Comments

@aomarks
Copy link

aomarks commented Oct 6, 2021

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

@aomarks aomarks added the feature Feature requests. label Oct 6, 2021
@claviska

This comment has been minimized.

@claviska

This comment has been minimized.

@aomarks

This comment has been minimized.

@KonnorRogers

This comment has been minimized.

@claviska

This comment has been minimized.

@justinfagnani

This comment has been minimized.

@KonnorRogers

This comment has been minimized.

@jaredcwhite

This comment has been minimized.

@justinfagnani
Copy link
Contributor

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

This comment has been minimized.

@claviska

This comment has been minimized.

@claviska

This comment has been minimized.

@claviska claviska added the in progress Currently under development. label Oct 13, 2021
@claviska
Copy link
Member

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
Copy link
Author

aomarks commented Oct 13, 2021

Thank you for the work on this!

@claviska
Copy link
Member

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

@claviska
Copy link
Member

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
Copy link
Member

Bug filed: jsdelivr/jsdelivr#18337

@claviska
Copy link
Member

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
Copy link
Contributor

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.

@claviska claviska added upstream An upstream issue is blocking the issue. and removed in progress Currently under development. labels Dec 31, 2021
@straversi
Copy link
Contributor

👋 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
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

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
Copy link

MartinKolarik commented Jan 26, 2022

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.

@claviska claviska added this to the v2.0.0 release milestone Feb 15, 2022
@justinfagnani
Copy link
Contributor

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.

@claviska claviska removed this from the v2.0.0 release milestone Nov 8, 2022
@claviska claviska removed the upstream An upstream issue is blocking the issue. label Nov 8, 2022
@claviska claviska added this to the 3.0 (future) milestone Nov 8, 2022
@rstoneIDBS
Copy link

rstoneIDBS commented Apr 11, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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 lit/lit#3770).

@claviska
Copy link
Member

@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.

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
9 participants