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

[Jetbrains] Add support for superclass #69

Closed
maicol07 opened this issue Jan 23, 2024 · 13 comments
Closed

[Jetbrains] Add support for superclass #69

maicol07 opened this issue Jan 23, 2024 · 13 comments

Comments

@maicol07
Copy link

Hi,

I've noticed some attributes are not added to the web types file: investigating the issue Iv'e found out the generator doesn't look into the superclass member (and therefore it doesn't add the superclass attributes which the elements extends.

Can you add it?

Thanks

Repo: maicol07/material-web-additions: An attempt to develop missing Material Web components (github.com)
custom-elements.json

@break-stuff
Copy link
Owner

Oh, interesting. The CEM Analyzer does this for you. I wonder if there is a way to include inherited properties using the Lit Anlyzer.

@break-stuff
Copy link
Owner

I asked about it on Discord and haven't heard anything back.

@maicol07
Copy link
Author

maicol07 commented Jan 29, 2024

Maybe adding something in the integration to recursively find attributes when superclass is found?

@break-stuff
Copy link
Owner

I'm reluctant to build this into the tool since based on the schema, this should be included in the CEM from the generator and would negatively impact performance. I would recommend using a tool like the CEM Analyzer which includes that feature or filing a bug with the Lit analyzer to see if they will include that feature.

@justinfagnani
Copy link

this should be included in the CEM from the generator

It's optional for generators to include members from a superclass. Consuming tools should look to superclass declarations. Those could conceivable change independently of the subclass, like if the superclass's manifest is updated on npm.

@break-stuff
Copy link
Owner

Are you referring to a superClass from an external CEM?

@break-stuff
Copy link
Owner

Also, do you know if this will be a feature included in the Lit Analyzer? I'm surprised it's not.

@justinfagnani
Copy link

Both from external or from the same package. The reason to not copy superclass members to subclasses is to not bloat the manifest.

I don't think this is a priority to add to the Lit Analyzer compared to ensuring coverage of other declarations like mixins, consuming external manifests, etc. The idea is that other CEM tools should be able to copy down or deduplicate members as necessary. ie, given a set of manifests that don't copy down inherited members, a tool can generate a new set that does.

I need to try to dig up the conversation on this and document this better in the schema.

@break-stuff
Copy link
Owner

I see what you're saying, but would it be an issue to bloat the manifest in this scenario? Not doing so would create a lot of extra work per tool. If I have 4 plugins generating content from the CEM, I have to resolve inheritance 4 times.

This also feels like a complex barrier for entry for tool authors because they may not have the appropriate context to resolve inheritance.

@break-stuff
Copy link
Owner

@maicol07, I just published this project that you should be able to use to help resolve this issue. Let me know if you run into any issues.

https://www.npmjs.com/package/custom-elements-manifest-inheritance

@maicol07
Copy link
Author

maicol07 commented Feb 12, 2024

Works! Thanks
The only issue I've found is that attributes are added as properties, but I think it's a lit analyzer missing feature: lit/lit#2993
(also here: https://github.com/lit/lit/blob/59d5ba7649f2957d01996e3115d0d79bdfbf34ac/packages/labs/gen-manifest/src/index.ts#L173)
(see below)

@maicol07
Copy link
Author

Just opened a PR in Lit repo to add this to the manifest generator: lit/lit#4547

@break-stuff
Copy link
Owner

Closing this for now. Let me know if you run into any other issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants