[labs/analyzer] add mixin support

[43081j]

Replaces #4120.

This is actually kevin's branch but caught up from main, and various tests added.

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 421d276b1e03ee3dbf90a9da8c0cda5d3cf1be39

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@lit-labs/analyzer	Patch
@lit-labs/gen-manifest	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[43081j]

i've also tried to update all occurrences of throw new DiagnosticsError or throw new Error to instead create/add a diagnostic and return undefined.

seems to be what we do elsewhere in the codebase, but do correct me if im wrong

edit:

i've also dropped a test we had for static methods since it seems like it belongs in its own PR and was failing for unrelated reasons (think i lost some of the fixture code in merge).

[43081j]

fyi i've marked for review but i have some tests to write still

there's also a comment or two i left myself which would be good to resolve first

[43081j]

FYI i've also now updated gen-manifest to support mixins.

i still need to update the goldens at some point, though.

[43081j]

ok as per discord, for now i've done two things:

1. drop isMixinClass so i can just figure it out from scratch (see note below)
2. only consider call expressions to be applied mixins if the identifier is a mixin

Regarding the isMixinClass stuff... it actually had a bug in it before anyway so i've decided to remove it entirely and just solve the problem from scratch.

The bug, if anyone's curious, was that we only set isMixinClass: true for ClassDeclaration. For custom elements and lit elements, we never passed the flag along. This meant they always had a superClass, while regular mixin class declarations didn't.

If anyone wants to help me figure it out, the aim is to make mixin classes have no superClass in their heritage (since it'd be the parameter to the function, nothing useful i guess). To do that, somehow we need to detect we're computing the heritage of a mixin class in getHeritageFromExpression and null off the superClass if so. We achieved that with the flag before, but I feel like there must be a cleaner way.

If it turns out there's no cleaner way, i'll go back to the flag.

I also added 3 new tests which fail right now and assert that for classes, custom elements and lit elements we set superClass to undefined for mixin classes.

[43081j]

ok for now at least i've reintroduced isMixinClass and fixed the bug with it

tests should pass now, PTAL if anyone gets chance

[github-actions[bot]]

The size of lit-html.js and lit-core.min.js are as expected.

[kevinpschaaf]

The changes LGTM, and thanks for tracking down the isMixinClass bug, the change to thread it through looks right to me.

It looks like we still need to fix the error of the Vue wrapper generator not walking the heritage chain to collect properties (and be robust to an element not having any properties).

So we don't lose the context, here's some notes I put in Discord a while back:

It looks to me like the type error is because the generated Props interface is an empty object with no keys (so p as keyof Props is always never).

It looks like Props is empty because it is just using declaration.reactiveProperties from here to generate that interface: https://github.com/lit/lit/blob/main/packages/labs/gen-wrapper-vue/src/lib/wrapper-module-template-sfc.ts#L80

So I'm guessing we (a) need to make/use a helper to collect reactive properties from the entire proto chain (not just the youngest class declaration in the chain), and (b) improve the Vue generation code to deal with the fact that it's actually valid for a component to have no reactive properties (which currently leads to the type error here) iow, the root cause is that ElementMixins itself has no reactive properties, it inherits them from the mixin, and we're not collecting all reactive properties to generate the view Props interface

[43081j]

#4293 might fix that. I opened it back when we discussed it, maybe worth merging that first. Will have to remind myself what I did there though

[kevinpschaaf]

Awesome, sorry I missed that. LGTM'ed, and once that merges, hopefully we can get this merged.

[augustjk]

With https://github.com/lit/lit/pull/4293 merged, perhaps this can be rebased and revisited? I know @e111077 has tried this on MWC and had it work for its doc generation.

[e111077]

Can confirm that this implementation worked on the MWC repo with a small fix to the MWC repo's generator which made some silly assumptions

[43081j]

👍 i'll rebase later today and see how it goes

[43081j]

rebasing was proving difficult which is why i ended up catch up merging last time i think, so i just did the same again

the tests still fail thanks to some typescript import stuff, not too sure what to do about that. all ts imports are the same, in files i've not touched too. so not entirely sure which import exactly is causing problems

[43081j]

looks like i got tests passing now 👍

[e111077]

BTW @kevinpschaaf has given this PR a verbal LGTM

[43081j]

just did the minor changes @augustjk suggested (some package name updates) FYI