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

Inconsistency between All package on CDN and NPM modules #1285

Closed
digitalsadhu opened this issue Dec 20, 2023 · 6 comments · Fixed by #1295
Closed

Inconsistency between All package on CDN and NPM modules #1285

digitalsadhu opened this issue Dec 20, 2023 · 6 comments · Fixed by #1295
Assignees

Comments

@digitalsadhu
Copy link

Which package(s) are affected?

Lit Core (lit / lit-html / lit-element / reactive-element)

Description

We've noticed a bit of an inconsistency between the Lit all CDN package and using the packages via npm. My expectation is that the CDN URL would essentially be a drop in replacement, however there's an edge case due to aliasing of html from the static-html package.

This:

import { unsafeStatic, html } from "lit/static-html.js";

and this:

import { unsafeStatic, html } from "https://cdn.jsdelivr.net/gh/lit/dist@3/all/lit-all.min.js";

are not the same since.

In the former you get html from the lit static package where as in the latter you get html from the core package.

This has proved quite a gotcha for us and as far as I'm aware isn't well documented.

If you'd like me to PR a documentation change, just point me in the right direction.

Reproduction

No reproduction should be necessary

Workaround

No workaround found.
This should probably be documented.

Is this a regression?

No or unsure. This never worked, or I haven't tried before.

Affected versions

All versions

Browser/OS/Node environment

Environment not relevant

@augustjk
Copy link
Member

Aha, that's a good point. Since the "all" bundle exports everything from a single entrypoint, the static version of html is aliased to staticHtml here https://github.com/lit/lit/blob/25fbfba9c0f1b97d720a981831c59c08472ba6ee/packages/lit/src/index.all.ts#L33-L39

Indeed this isn't documented anywhere in https://lit.dev/docs/getting-started/#use-bundles
There's an edit this page link on the bottom which points to this file https://github.com/lit/lit.dev/blob/main/packages/lit-dev-content/site/docs/v3/getting-started.md which could have a note that the static versions of html and svg are aliased to staticHtml and staticSvg.

Personally, I'm less sold on the need for our bespoke bundles when CDNs can serve npm published packages and their submodules directly like

import { unsafeStatic, html } from 'https://esm.run/lit@3/static-html.js';

which is more of the replacement you were expecting.

And if you really need it all in a single js file, esm.sh has that feature too though it can cause duplicate code to be loaded.

import { unsafeStatic, html } from 'https://esm.sh/lit@3/static-html.js?bundle-deps';

@digitalsadhu
Copy link
Author

Personally, I'm less sold on the need for our bespoke bundles when CDNs can serve npm published packages and their submodules directly like

I can see that. Deprecate the all bundle?

And if you really need it all in a single js file, esm.sh has that feature too though it can cause duplicate code to be loaded.

This is the unfortunate part for us. We actually have developers use the NPM package version and then use server side import mapping at build time to map all uses to the 1 bundle that we produce and publish to our CDN from the Lit NPM package. Since html will always clash between the 2 packages, we have now realised we need to produce 2 bundles and eat the duplicated code cost since we can't map developers use of html to staticHtml in the all bundle. At least not as it currently stands, we can only import map bare imports to absolute urls.

So our new solution looks like this:

// this gets mapped
import { LitElement, css, html } from 'lit';
// to something like:
import { LitElement, css, html } from 'https://assets.finn.no/npm/lit-3/v3/lit.min.js';

and

// this gets mapped
import { unsafeStatic, html } from 'lit/static-html.js';
// to something like:
import { unsafeStatic, html } from 'https://assets.finn.no/npm/lit-3/v3/static-html.js';

Which costs us IIRC about 7kb extra.

@augustjk
Copy link
Member

It's not huge but certainly unfortunate none the less.

So if I'm understanding this, you aren't modifying any of the specifiers in the consumer code which is why the alias is problematic.

Trying to orchestrate might not be easy but is it possible that your build for static-html.js could not bundle everything and instead refer to your lit.min.js for what it needs? It really only needs the original html and svg tags https://github.com/lit/lit/blob/main/packages/lit-html/src/static.ts

Maybe a build step that first builds the lit.min.js as a bundle, and another build step that points to the static.ts file above as entrypoint and all it does is modify the import {html as coreHtml, svg as coreSvg, TemplateResult} from './lit-html.js'; so that it resolves to the first build's artifact (or replace './lit-html.js' with 'lit' so the importmap will do that) and minify, without bundling? Perhaps a custom esbuild plugin could rename and mark it as external.

@digitalsadhu
Copy link
Author

Happy New Year!

That could work. The only downside I can see is that there's an additional waterfall load in there though if you're not using static you wouldn't care and if you are using static you'd use it directly and would preload it so it likely doesn't actually matter I guess.

Thanks for the suggestion!

@digitalsadhu
Copy link
Author

(or replace './lit-html.js' with 'lit' so the importmap will do that)

Actually, I can probably just define a map of ./lit-html.js to https://assets.finn.no/npm/lit-3/v3/lit.min.js and run a build and build a version of static with just that.

@augustjk
Copy link
Member

augustjk commented Jan 4, 2024

Hope that worked! I've transferred the issue to lit.dev repo as we should document the aliases where we mention the bundle.

@augustjk augustjk changed the title Inconsistency between All package on CDN and NMP modules Inconsistency between All package on CDN and NPM modules Jan 4, 2024
@augustjk augustjk moved this to 📋 Triaged in Lit Project Board Jan 4, 2024
@augustjk augustjk self-assigned this Jan 8, 2024
@github-project-automation github-project-automation bot moved this from 📋 Triaged to ✅ Done in Lit Project Board Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants