@storybook/web-components lit v1 compat requires lit-html dependency

[43081j]

Describe the bug

Introduced by #14898.

The peer dependency configuration means that we're forced to have lit-html as a dependency to make storybook pull in the right peer.

We have something like this inside storybook now:

  "lit-html": "1.4.1 | 2.0.0"

however, going forward the best practice is to pull from lit, not lit-html. Which means all lit v2 projects will have to install lit-html as a dependency to force storybook into resolving the right one.

Not having the right version means we then have the wrong TemplateResult in the types, causing build errors in typescript projects.

you can see this also mentioned here:

https://github.com/storybookjs/storybook/pull/14898#issuecomment-847694008

I also understand we're making @storybook/lit but web-components should still work correctly, without having to add an unnecessary dependency.

[shilman]

@43081j what's the backwards-compatible fix? we'll probably drop v1 support in SB 7.0, but in the meantime what to do? make it an optional peer dependency?

cc @Westbrook @gaetanmaisse @brion-fuller @merceyz

[43081j]

im really not sure, its a super awkward situation.

i think we should have set them both as optional peer dependencies (lit-html 1.x, lit 2.x). im not sure how npm would handle the resolution, but maybe then if you had lit 2 as a peer, it'd resolve the lit-html import to the one inside lit 2.

if thats not how npm would deal with it, we'd be a bit stuck i think

i'd hope, us having lit 2 would result in:

- lit@2
  - lit-html@2
- @storybook/web-components
  - lit@2 (deduped)
    - lit-html@2 (deduped)

but no clue if thats how npm would do it

that'd mean we could still import from lit-html even though its a transitive dependency.

[merceyz]

It doesn't look like its optional so as it is right now it needs to stay required https://github.com/storybookjs/storybook/pull/14898/files#diff-6c3ee5d62a4cb906cba13eff0e50822ea7b155558e7df3a1d98cdf3403a51a00R3

that'd mean we could still import from lit-html even though its a transitive dependency.

You can't rely on that, see https://yarnpkg.com/advanced/rulebook#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies for more details

[43081j]

yeah which is why this is such an awkward position.

i suppose the "right thing" to do would've been to not support lit2 in 6.x, and not support lit1 in 7.x.

as far as i can tell, the issue is that consumers will install lit, which transitively gives them lit-html. but a transitive dependency doesn't fulfil a peer dependency requirement. which is why we then have to install lit-html for the sole purpose of making storybook happy.

im not sure there's any good solution to that, though. anything we do to support both at once will result in trouble/hackery i think

[ndelangen]

Do you think we should drop support for lit1 in storybook 7.0 beta? https://github.com/storybookjs/storybook/blob/067d3379b33754a80f996046c9f71df529ddb798/code/renderers/web-components/package.json#L72

And thus only support lit2?

[markcellus]

Whether to support lit 1 or 2 may need to be a different issue, I think. As I read it, this issue is about resolving the transitive dep discrepancies.

[Westbrook]

Removing lit-html@1.0 support in v7 will solve this and is a good idea.

Too bad CSF@3 went out without a renderer method that would allow for pluggable renderers, which would have prevented the need for SB to manage dependencies like this.

[ndelangen]

@Westbrook I can open a PR changing the peerDep restriction, easy, but perhaps it's something you'd like to do?

[43081j]

there's still a fair amount of projects around using lit 1.x with storybook im sure (think we have a few somewhere even).

however i agree w/ westbrook it'd be sensible to remove support for it and just keep those users on 6.x of storybook.