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

[WIP] Implement new experimental JSON schema #157

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[WIP] Implement new experimental JSON schema #157

wants to merge 3 commits into from

Conversation

runem
Copy link
Owner

@runem runem commented Mar 27, 2020

This PR implements the proposed JSON schema found here as a new experimental output format called json2.

I will have to make some more changes to the internal data structure in order to better output this schema.

You can test it at the playground here: https://runem.github.io/web-component-analyzer/?format=json2. I will make sure to update the playground when I finish more parts of the schema.

You can also clone this repo, build it and run ./cli.js ./dev/src/custom-element/custom-element.ts --format json2. I might publish as beta version on NPM soon.

While implementing this I wrote down some questions and considerations which I will add to the discussion at webcomponents/custom-elements-manifest#9 during this week :-)

@runem
Copy link
Owner Author

runem commented May 9, 2020

Here is an update of what TODO's are still needed on my side:

There are still some things that have not been implemented yet, specifically:

TODO: (updated to reflect newest changes to schema.ts)

  • All paths should be relative from root, but paths to non-project/external modules/packages/dependencies should just use the import specificer directly.
  • Publish beta version of the CLI and update the playground
  • Use the summary JSDoc tag to fill out "summary"
  • mixins on ClassDeclarations
  • parameters and return on ClassMethod
  • static on ClassField and ClassMethod
  • FunctionDeclarations and MixinDeclaration in the declarations array on JavaScriptModule
  • type on Event
  • demos on CustomElement

I will be focusing a bit more on lit-analyzer the next couple of weeks until I have a 1.2.0 release, so I'm not sure how much progress I will be able to make on web-component-analyzer during the next week or two :-)

@runem runem force-pushed the json2 branch 2 times, most recently from 6f108e5 to c768a8f Compare June 13, 2020 21:51
@runem runem force-pushed the master branch 2 times, most recently from ad0910b to d802a91 Compare July 12, 2020 18:27
@thepassle
Copy link

Introduce a type to the exports array to be used when a custom element is defined in a given module and remove tagName from CustomElementDoc. For example:

I think the CustomElementDefinitionDoc is a good idea, but I think by default a classdoc shouldnt be dependant on a tagname, right?

@runem
Copy link
Owner Author

runem commented Nov 13, 2020

I implemented most of what's in the updated schema. While implementing the changes, I wrote down a couple of questions that I would like to discuss.

  1. What is the current schemaVersion field on Package? I assume that we are using semver versioning, so I just set it to 0.0.1
  2. The fields package and module are optional on the type Reference. What does it mean if these are not included? Does it make the reference global (such as "HTMLElement"), or does it mean that it references the same module as the module of the symbol that referenced it? Or both?
  3. The field cssProperties on CustomElement is prefixed with css while parts isn't. Is this intended?
  4. The type Module isn't exported from schema.ts right now (line 47)
  5. How do I include unnamed classes in the output such as customElements.define("my-element", class extends HTMLElement { }) or a mixin returning an unnamed class? The declaration doesn't have a name, so it becomes impossible to reference and include it in the output.
  6. Why is the type field on Event required?
  7. Are mixins included twice the the declarations array as both MixinDeclaration and a FunctionDeclaration?
  8. MixinDeclaration extends both ClassLike and FunctionLike. Does this mean that a MixinDeclaration includes the fields on the class returned by the mixin? If so, this structure wouldn't be able to contribute with custom-element-specific fields such as attributes. If this is by design, is it the idea that the field return.type on MixinDeclaration needs to reference a CustomElement type declaration instead?

Right now, WCA doesn't properly include mixins in the output because it must be extended to analyze and understand mixins in another way. Mixins are only analyzed while traversing inheritance starting from custom element class declarations, but should be extended to analyze all mixins tagged with the @mixin JSDoc tag.

@thepassle
Copy link

thepassle commented Nov 13, 2020

What is the current schemaVersion field on Package? I assume that we are using semver versioning, so I just set it to 0.0.1

I think it should still be 'experimental', but we should really be aiming for 1.0.0 soon

The fields package and module are optional on the type Reference. What does it mean if these are not included? Does it make the reference global (such as "HTMLElement"), or does it mean that it references the same module as the module of the symbol that referenced it? Or both?

If the type is local in the current module, then the module field should point to the current module.

The field cssProperties on CustomElement is prefixed with css while parts isn't. Is this intended?

I think we should align in this case and name it cssPart, the type is also CssPart. Can you make a PR?

The type Module isn't exported from schema.ts right now (line 47)

Can you make a PR

How do I include unnamed classes in the output such as customElements.define("my-element", class extends HTMLElement { }) or a mixin returning an unnamed class? The declaration doesn't have a name, so it becomes impossible to reference and include it in the output.

Perhaps we should use a convention like AnonymousClass1, AnonymousClass2, etc for this specific case?

Why is the type field on Event required?

Because events can have types

Are mixins included twice the the declarations array as both MixinDeclaration and a FunctionDeclaration?

If its a mixin it should be a MixinDeclaration.

MixinDeclaration extends both ClassLike and FunctionLike. Does this mean that a MixinDeclaration includes the fields on the class returned by the mixin? If so, this structure wouldn't be able to contribute with custom-element-specific fields such as attributes. If this is by design, is it the idea that the field return.type on MixinDeclaration needs to reference a CustomElement type declaration instead?

Im not sure what you mean here, can you elaborate? The Mixins fields should be applied to the class that uses the Mixin, right? Not the other way around

@justinfagnani
Copy link
Collaborator

Thanks for starting the implementation @runem!

@thepassle's answers are great here. A few things:

  • I made a PR to export Module

  • I'll make a PR for parts

  • I think 0.0.0 is probably good as a version until we make sure there's minimal usability, and then go to 0.0.1 right away. Probably after we fix a few things here.

  • Something like AnonymousClass1 sounds good, but I think we should probably prefix it with a $ to make it look weird. We should give guidance on this in the schema docs so it's more likely to be consistent.

  • I see the point about mixins. In Polymer Analyzer we had modeled mixins as if they were a first-class JS construct, and I'm a bit wary of this approach, so I was trying something different and definitely missed being able to mix-in custom element features. Coincidentally, what we theoretically want is for 'CustomElement' to be a mixin that can be applied to ClassDeclaration. Since this is a custom-elements specific format, I think we can safely just have MixinDeclaration implement the custom element fields. I'll make a PR for that.

    As you hint at, another way to model mixins would be simply as functions that return a ClassDeclaration as a type. The problem I see there is that so far the types in this file are purely textual, with an allowance for spans being references. To have a function that returns a CustomElement subclass be specially recognized as a mixin, we'd have to specify some syntax/semantics for types.

    Another approach would be to have Mixin extend Function, but with one additional field that points to a declaration that's mixed in. That field could be a reference, so we don't care about the type syntax. We'd have the anonymous class issue here quite often.

    I'm a little unsure here, so I would love implementation feedback too, from both analyzers and documentation viewers. I think this is the biggest thing to finalize before the format is stable. I'll start with a PR for making Mixin inherit from CustomElement, and we can go from there.

@justinfagnani
Copy link
Collaborator

@runem I just made a PR for mixins that includes a comment on how to structure them: webcomponents/custom-elements-manifest#31

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

Successfully merging this pull request may close these issues.

3 participants