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

Initial schema #9

Merged
merged 14 commits into from Oct 26, 2020
Merged

Initial schema #9

merged 14 commits into from Oct 26, 2020

Conversation

justinfagnani
Copy link
Collaborator

The schema is authored in schema.ts and transformed into schema.json. Including both for review.

@justinfagnani
Copy link
Collaborator Author

cc @daKmoR @rictic @runem @octref

@daKmoR
Copy link

daKmoR commented Nov 11, 2019

what is a demo json for this?

* The attributes that this element is known to understand.
*/
attributes?: AttributeDoc[];

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having no "flattened" properties here makes is severely harder to list them for documentation?

e.g. I need to go to the class get all members and mixins and construct it myself... quite cumbersome :/

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would also prefer a flat version of custom-elements.json. Moving the complexity to the "builder" of the file instead of the "consumer".

But maybe there is a use case that would change my mind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely do flattened properties. In the Polymer Analyzer we flatten, but in each member indicate where they originated from: https://github.com/Polymer/tools/blob/master/packages/analyzer/src/analysis-format/analysis-format.ts#L327

We can do the same here, though I think that should be expanded from a string to a reference type.

This allows us to show where members are inherited from in the documentation viewer UI. Examples here: https://polymer-library.polymer-project.org/3.0/api/polymer-element

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best of both worlds, I like that

* The shadow dom content slots that this element accepts.
*/
slots?: SlotDoc[];

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

misses css custom properties right?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are now exports of the class? seems kinda like a stretch as to have css variables as part of javascript class exports?

also seems same problem as with not having flattened properties... tough to work and imho also to understand 🙈

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add the styles section. This is in an interface that extends a ClassDoc, so ti has everything classes do plus custom element specifics.

@daKmoR
Copy link

daKmoR commented Nov 11, 2019

overall this seems to move all the important parts to a full description of the javascript class... with inheritance and all.

which opens a whole lot more questions? what happens if we extend/mixin from an external node_modules package? do we then add this info to the custom-elements.json as well? e.g. will there be a full dependency/inheritance graph inside this file?
what if multiple components extend/mixin the same base class... then the analyzer needs to be smart enough to deduplicate and normalize those inheritances...

so single file analysis is then out of the picture 😭
e.g. analyzing 2 files independently and then merging the json will no longer work :/

analyzer my-foo.js -o my-foo.custom-elements.json
analyzer my-bar.js -o my-bar.custom-elements.json

Merging my-foo.custom-elements.jsonand my-bar.custom-elements.json will now lead to duplicate class data...

this will also hinder to move forward with the possibility to allow defining individual meta files per custom elements (think that every element in a repo has my-el.ce.json or so and then you can just merge it to create a "normal" custom-elements.json at the root of the package)

I really loved the simplicity of the format that this is my tag name and here are all my properties, attributes, ... and you don't need to know where they come from. (optionally we can add a class mixin/hint to help for the documentation but that's it)

Overall I would not see this as a goal of custom-element.json to describe the javascript classes with inheritance and mixins... if that is needed it should be a separate more detailed file imho...

custom-elements.json should only care about the custom elements and their attributes/properties/events/... and not about the implementation detail on how it offers these functionalities.

PS: this also leave is more open for other web-components libraries like haunted, stencil, ...

@georges-gomes
Copy link

Thanks Justin and Thomas to kick this off 👏

I'm intimidated with the details of class and mixins. I'm afraid of the adoption friction this could create.

I would stay on the "outside shell" description of the custom-element.
Attributes, props, slots, events, css properties ... I would add functions to these but only the ones that the author decided to expose to user.

I would also favor a flat description that is easy for the "consumer"-side of custom-elements.json.

My 2 cents

@georges-gomes
Copy link

BTW the authoring of the schema in ts is really nice - much easier to read than the output schema 👍

/**
* A markdown description of the slot.
*/
description?: string;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe there should be a property to define allowed content?

@thepassle
Copy link
Collaborator

Im with George on this, from my point of view, im seeing types and custom-elements.json as a separate thing. For example, for the extension I'm working on: all I need to know for the 'tweak' functionality is what the attributes are, what the properties are. Then I need some type information like: str, bool, number, object, array. Thats all I need. I think we should aim for that, and then if people want to, they can hook up their typescript definitions file somehow that could give extended information (eg: its an object, but this is what the object looks like, or: its an array of Products or whatever)

Imo custom-elements.json should not try to solve typing and I see it as a (not entirely, but somewhat) separate concern

@justinfagnani
Copy link
Collaborator Author

I think we need to consider the full set of use cases for this data. Documentation viewers, like webcomponents.org will need to show both the HTML-side and DOM/JS-side of element interfaces. Because elements are classes, this means showing properties and their types, methods and their parameter and return types, etc. As properties and methods accept complex object types, we need to document them too. Libraries like LitElement/lit-html will need to document arbitrary things like variables and functions.

We had a lot of success with the Polymer Analyzer providing enough information for documentation viewers and using the same data for IDE integrations where type may also be provided by TypeScript or other analysis.

I really don't think types are separate here. If TypeScript provided a standard JSON format for types, or if JSDoc had a standard JSON format for analysis result, maybe, but they don't. The serialization of type information into an easily consumable format, unlike .d.ts files, is a huge advantage to tools based on this format.

@georges-gomes

I'm intimidated with the details of class and mixins. I'm afraid of the adoption friction this could create.

I don't think this will negatively affect adoption at all, but is more likely to increase it. There are only two ways that this file will be produced: by hand or with a tool. A tool is going to have no problem filling in the details allowable here, and a file written by hand can just omit almost everything.

For example, this is a valid file under this schema:

{
  "schema-version": "2.0.0",
  "modules": [{
    "path": "",
    "exports": [{
      "kind": "class",
      "name": "MyElement",
      "tagname": "my-element",
      "superclass": "HTMLElement"
    }]
  }]
}

This defines a class with no additional properties on top of HTMLElement. Even if you used mixins to define properties, you could just document the properties and leave out the mixins. The point of having optional mixins field is to allow for documenting the mixins for use with documentation tools that want to do cross-references, etc. The presence of the option doesn't increase complexity for authors or tools that ignore it.

@justinfagnani
Copy link
Collaborator Author

@daKmoR

overall this seems to move all the important parts to a full description of the javascript class... with inheritance and all.

Exactly, because this reflects the reality of web components. You cannot accurately document a web component without documenting the JavaScript class it's implemented with.

which opens a whole lot more questions? what happens if we extend/mixin from an external node_modules package? do we then add this info to the custom-elements.json as well? e.g. will there be a full dependency/inheritance graph inside this file?
what if multiple components extend/mixin the same base class... then the analyzer needs to be smart enough to deduplicate and normalize those inheritances...

These questions exist whether the schema provides answers or not. What does happen when we extend from a class in another package? Without this information representable in the schema, the answer is that we can't even express this common situation. With space for this information we can, even if some tools don't produce it.

so single file analysis is then out of the picture 😭

It already is in practicality. A tool that analyzes components built with LitElement really should be analyzing LitElement itself to copy down properties and methods from it. Not doing that will create a very incomplete picture of the element's API.

e.g. analyzing 2 files independently and then merging the json will no longer work :/
Merging my-foo.custom-elements.json and my-bar.custom-elements.json` will now lead to duplicate class data...

I don't see why this would be true. The format organizes APIs by module, you will be able to concatenate the module lists from multiple packages with no duplication.

If you are analyzing multiple modules in the same package, you should absolutely be producing a single file. I don't think we've discussing having multiple custom-element.json files per package.

this will also hinder to move forward with the possibility to allow defining individual meta files per custom elements (think that every element in a repo has my-el.ce.json or so and then you can just merge it to create a "normal" custom-elements.json at the root of the package)

We really need to discuss this in use cases if this is something you want to pursue. How would tools find all these files? Right now a tool can just look for the presence of a single well-known file name. If we have multiple, open-ended filenames, tools will need to know how to find and collate them. That would definitely be an increase in compelxity.

I really loved the simplicity of the format that this is my tag name and here are all my properties, attributes, ... and you don't need to know where they come from. (optionally we can add a class mixin/hint to help for the documentation but that's it)

That's totally supposed within this format.

Overall I would not see this as a goal of custom-element.json to describe the javascript classes with inheritance and mixins... if that is needed it should be a separate more detailed file imho...

custom-elements.json should only care about the custom elements and their attributes/properties/events/... and not about the implementation detail on how it offers these functionalities.

There is no current other file format for the JavaScript side of things, and there is no clear line between elements and JavaScript. Even documenting properties already crosses over from pure HTML-side API docs and into JavaScript docs. As soon as you have a property that accepts a complex value, you need to document that value. If it's a function-typed value, you need to be able to document arbitrary functions. When you try to document components along with the libraries they're built-with, you need even more JS documentation abilities.

We have a lot of experience trying to provide these tools for web components, and it's a basic truth of the problem that documenting web components is a superset of the problem of documenting JavaScript. Web components are just JavaScript objects with additional API surfaces.

I think trying to separate things will lead to a much worse developer experience, where the documentation for a single element is spread across two formats and two viewers. Like one for attributes, and a JSdoc/TSdoc one for the JS API. That would be very unfortunate.

PS: this also leave is more open for other web-components libraries like haunted, stencil, ...

I don't see how allowing a more complete description of components would not allow for all possible web component implementation libraries. There's nothing about Haunted or Stencil that wouldn't work here.

@daKmoR
Copy link

daKmoR commented Nov 12, 2019

with single file analysis I meant analyzing one web component 🙈 (it will still need to follow all its imports) but that you can analyzer each web component independently and that you later do a straight forward merge to get all the information (possible as each tag/web component only adds one item to the tags array)

I don't think custom-elements.json should replace documentation tools like http://typedoc.org/. They just offer a whole different information density - something that is in my experience rarely need for users of web components. Most cases I know of Users are interested in the tag name a few properties/css Properties and some events...
If they need waaayyy more then code or typedoc is probably a better place to start looking

Most design system I could find document only the important properties/events (if even) as giving all the info is rarely what the user needs...?
Examples:

If we could provide the values used in the examples (usually name, description, type, default) we are already golden 🤗
Right now I would not overvalue addition details (which can be nice but imho are a nice to have)

I, however, see that we have different points of view on this matter - let's see what the other guys are saying.

@daKmoR
Copy link

daKmoR commented Nov 12, 2019

@rictic @runem @octref we need your input here 🤗

@justinfagnani
Copy link
Collaborator Author

@daKmoR the first like I clicked on above was the Gestalt link. This is a React library, so it has a lot less to document as React components don't typically have methods, nonetheless, I see several problems that would be solved by fuller documentation.

The SearchField component has property types that are complex JavaScript object: https://pinterest.github.io/gestalt/#/SearchField like SyntheticEvent<HTMLInputElement>

Those are referenced by name only and no additional information is available about them. There are several data types that are documented inline, which is fine if they're simple but won't scale to references to more complex interfaces or shared classes.

The first link in the list shows some problems too. https://design-system.pluralsight.com/components/button#proptypes

It references some enum values in the prop types, but has to expand out each enum and just say in the text things like "visual style (from Button.appearances)" because it apparently lacks the ability to directly document Button.appearances.

So even though these are React components and have a limited API surface, the docs could be improved with general JS/TS doc abilities.

Again, tools don't have to use these fields or generate these fields. They're optional. A Storybook generator could ignore many things if it didn't have a place to put them. An analyzer for a specific library can not generate them. But for LitElement we definitely will want to produce analysis results for the full APIs as we have done for Polymer. So we'll definitely end up with some file format with these capabilities. I'd rather not have two formats.

@daKmoR
Copy link

daKmoR commented Nov 14, 2019

I have another idea 🤗

How about combining and linking it?
So we can keep the simplicity for certain use cases and have more details when needed?

Basic idea is to have both

  • tags (as flattened data)
  • modules (as nested data)
    at the root level.

Where tags would be a flat list of attributes, properties, methods, ...
=> use case would be VS Code autocompletion, "simple" docu for design systems or storybook, ...

And modules would be the full module exports "tree" including mixins and separate properties/methods/... list for each "prototype".
=> use case would be for full (api) docu, analysis of usage, show dependency trees, ...

TagDocs would have a class which links to the correct module export
TagPropertiesDoc would have a "class/mixin" which links to the correct module export

it could look something like this (just psoido code 🙈 )

export interface PackageDoc {
  version: number;
  tags: TagDoc[];
  modules: Array<ModuleDoc>;
}

export interface TagDoc {
  name: string;
  class?: ClassDoc,
  attributes?: TagAttributeDoc[];
  events?: TagEventDoc[];
  slots?: TagSlotDoc[];
  cssCustomProperties?: TagCssCustomPropertyDoc[];
  cssShadowParts?: TagCssShadowPartDoc[];
  properties?: TagPropertyDoc[];
  methods?: TagMethodDoc[];
  demos?: TagDemoDoc[];
}

export interface ModuleDoc {
  path: string;
  description?: string;
  exports?: Array<ExportDoc>;
}
export type ExportDoc = ClassDoc|FunctionDoc|VariableDoc;

export interface ClassDoc {
  kind: 'class';
  name: string;
  description?: string;
  superclass?: Reference;
  mixins?: Array<Reference>;
  members?: Array<ClassMember>;
}

// ...

@georges-gomes
Copy link

Should we separate the concerns?

  • tag documentation in custom-elements.json
    name, attrib, props, events, slots, css*, demos...
    Description from HTML perspective (ok props are negociable)

  • class documentation in custom-elements.d.ts
    This is how you interact with it from a js/ts perspective.
    The IDEs would like that a lot I think. (so do I)

The first one can point to the types of the second one.

Makes sense?

@justinfagnani
Copy link
Collaborator Author

justinfagnani commented Nov 15, 2019

I don't think there's as clear of a split as it seems, indicated by the fact that some people seem to want to put properties on the custom element side of things. Properties are part of the JS interface. This shows that the JS interface is important :)

Also, .d.ts files are not easily parsable. You need to bring along the whole TypeScript compiler, so writing tools that collate the custom element metadata with the JS metadata are going to be much more complex. Then there's the question about what to do about the fields that are duplicated between the two sources, like for aforementioned properties.

Splitting the information across two files is strictly more complex than keeping them in a single file.

I want to point out again that the JS-related fields are optional. If you don't want to generate them, don't. If you don't want to read them, don't.

@justinfagnani
Copy link
Collaborator Author

@daKmoR

Basic idea is to have both

tags (as flattened data)
modules (as nested data)
at the root level.

Custom elements are classes. So if we split their description, then tools will just need to recombine it. Let's say we have my-element-1 and my-element-2 in two different modules in a package. We'll have to describe what modules those elements are in in the "tags" section. Then a tool will have to pull the tags out of that section, associate them with the modules documented in the "modules" section, and merge their information with the appropriate classes in there. For what benefit?

I think we can derive the appropriate schema by following the ground truth of the actual custom element definitions. Custom elements are defined in modules, modules are contain in packages. So a package needs to document it's modules, and the custom elements they contain. It's much easier to go from package>module>element and find all the elements with their complete interfaces, than the other way around.

@georges-gomes
Copy link

@justinfagnani
First, I'm completely onboard with you on the class definition now. You convinced me.
I can see how much better the developer experience could be with this information.

But, I'm a little bit afraid of reinventing the wheel by not adopting typescript d.ts for class definition.
The killer use case for class definition is IDE integration right? If it's the case, then they already have typescript loaded and ready to use. IDEs already read all type definition and jsdoc description... it's a lot of work to reimplement in a new format in both "our" side and on the side of the IDEs.

I gently disagree with the fact that it would be more complex to use .d.ts files instead of new specific format.

But maybe there are other use cases of the class definition that I don't see today.

@justinfagnani
Copy link
Collaborator Author

The .d.ts files are still the best source for IDEs. In that case, only the additional custom elements information would likely be used, as the rest of the JS side would be derived from the .d.ts files or .ts files. Note that this is true of the property side of custom elements as well, even if there were no additional JS documentation.

IDEs are only one use case for this file. One critical use case we have for the webcomponents.org catalog and custom design system catalogs is end-user documentation of the entire element interface. For this we need the JS docs, and we'd really like everything to be in an easily consumable JSON file, rather than having to spin up a TypeScript parser to get the JS docs, and then collate the information back together with the custom elements data.

In the case of an analyzer like web-component-analyzer, it's already directly consuming TS/JS and is generating data based on the classes it finds. So the custom element / JS information is unified at the source. To split this information up into separate files, only to recombine it later is only more work and more dependencies to involve.

@georges-gomes
Copy link

ok I buy that 👍

Can we start by settling the tag part of the file: Attribs, props, slots, events, demos, cssprops? And then extend it with the class data in a second step?

So much could be unlocked with just this first step if we can converge into something official.

The catalog and an extension of open-wc, the web-component-analyzer and our support in webcomponents.dev are just the projects I know that could move forward if we have just that first step.

But I state the obvious here I know :)
Happy to help in any shape or form

@georges-gomes
Copy link

@justinfagnani
what do you think of the proposal of @daKmoR ?

export interface TagDoc {
  name: string;
  class?: ClassDoc,
  attributes?: TagAttributeDoc[];
  events?: TagEventDoc[];
  slots?: TagSlotDoc[];
  cssCustomProperties?: TagCssCustomPropertyDoc[];
  cssShadowParts?: TagCssShadowPartDoc[];
 ( properties?: TagPropertyDoc[]; )
 ( methods?: TagMethodDoc[]; )
  demos?: TagDemoDoc[];
}

This way we have the "HTML shell" that is available in a flatten format and then we have class to move to the class level.

On the basis that the class definition stays manageable, we could have properties and methods into the class definition, I agree 👍

@octref
Copy link
Contributor

octref commented Nov 18, 2019

@justinfagnani

I really don't think types are separate here. If TypeScript provided a standard JSON format for types, or if JSDoc had a standard JSON format for analysis result, maybe, but they don't. The serialization of type information into an easily consumable format, unlike .d.ts files, is a huge advantage to tools based on this format.

You might want open an issue in TSDoc for discussion. They already generated an AST from TSDoc, and I believe that can be easily converted to JSON.

Re putting types into the JSON file: I don't think TypeScript would interoperate with it. I think the best way forward is to provide tools to convert custom-elements.json into multiple format, each for specific purposes. This way you can continue make changes to the schema while making features in different areas work as intended:

  • Generate Custom Data, so HTML / CSS language features work in LSP supported editors
  • Generate Web Types, so HTML / CSS language features work in JetBrain editors
  • Generate d.ts, so TypeScript language server could handle it
  • Generate a HTML presentation on webcomponents.org, so people can read it

@runem
Copy link

runem commented Apr 2, 2020

Hi all :-)

I'm in the process of implementing this schema as an experimental output for web component analyzer so that we can explore it. You can follow my progress in this PR, and you can play around with it in the playground 🤗

During implementing the schema, I wrote down some questions/thoughts that I would like to add to this discussion. I see that there have already been some discussion around most of my points, and I could have been overlooking something, so please correct me where I'm wrong.

1. Flatten

My first question is about the flattening of members. There has been a lot of discussion around how to do it in this thread, but I'm not sure about what/if we landed on anything here. Therefore, the schema I implemented doesn't have flattened members. I'm looking for guidance/a discussion on this point.

2. Tag name vs class declaration

I'm a bit unsure of what to output when the customElements.define is in module A and the actual class declaration in module B. In the schema there is no seperation between tag name and class, so in the JSON the entire tag (CustomElementDoc) would have come from one of the modules. One could argue that the CustomElementDoc in this case should come from module A because this is where the tag name is defined, and one could argue that it should come from module B because that's where the class is exported. Is there need for seperation between the declaration and tag name in the schema, or are we okay with this? And in that case, what would the most valid answer be to the above mentioned?

3. Inherited custom element specific

The current schema has a required tagName on CustomElementDoc and optionally "events", "slots", "attributes", "cssProperties" and "cssSlots" arrays (custom element specifics). In practice, any class in the inheritance chain could contribute to the custom element with events, attributes eg., but it seems like this isn't currently possible to model in the schema, because custom element specifics only exist on the same data structure as the required tagName. I think it would be an advantage if it was possible to easily lookup for example a MixinDoc in the exports array on ModuleDoc and see exactly which events, attributes, eg. it contributes with in addition to its members (properties and methods). I'm aware that the reason for this is that ClassDoc only includes the javascript class interface, and not custom element specifics.

I want to raise this point, because an effect of (1) not including custom element specifics on the ClassDoc AND (2) not flattening, is that inherited custom element specifics are currently not included in the output for WCA. Only custom element specifics directly from the class defined as a custom element will be included. It could be solved by including (flattening) all inherited features on the CustomElementDoc in the output, but this wouldn't make it possible to easily lookup everything a MixinDoc contributes with.

4. Non-exported classes in the inheritance chain

This might be an edge case, but I'm including this question anyway. Only exported classes are included in the exports array on ModuleDoc. However, if a class in the inheritance chain is not exported from the module, it won't be included in this array. This would make it impossible to follow the inheritance chain. Example:

class A {}

// Only this class would be included in the exports array on ModuleDoc
export class B extends A {}

Would we be fine with this behavior (eg. it's up to the author to export all classes in the chain)?

@justinfagnani
Copy link
Collaborator Author

Awesome news and awesome work @runem! Can't wait to try this for the element I was using my fork on.

You raised some really good points. My initial thoughts:

Flattening

I think flattening makes a lot of tools that consumer these files easier to build, especially when considering cross-package inheritance. As raised in the last meeting, this does have some issues around freshness, if a dependency has been updated, but I think more sophisticated tools that can consumer multiple packages can ignore inherited members if they have access to the originals.

You point about anonymous classes might weigh in on this too. The easiest thing might be to say that they're flattened, but we don't have a reference to the class they inherited from.

Tag name vs class declaration

This is a good point, especially when some people want to separate registration and class declaration into separate modules. I now think that tag names should be a distinct type of export for a module that reference their class declaration. Because there's a 1-1 relationship, the class should have a reference back to the name if there's a define() call. There are some possible edge cases where a class has multiple possible define calls, but I expect that to be extremely rare-to-non-existent.

Inherited custom element specific

Any class or mixin should be able to have custom element fields. We'll probably need some kind of field that says it has custom element data mixed in. Custom element fields should flatten if class members do.

Non-exported classes in the inheritance chain

I don't think this is going to be a completely rare edge case. It seems like there are only two options: 1) "export" an anonymous class, or 2) flatten the members into the nearest exported subclass. I think we could always flatten class members, but make a reference to where they were inherited from optional for anonymous superclasses.

@runem
Copy link

runem commented Apr 23, 2020

Thanks for your answer Justin! :-)

In the newest version of the WCA playground, mixins and superclasses are included, and all features have been flattening. This required quite a bit of refactoring. You can see it in action here. However, I haven't yet added TagNameDoc to the output, and I don't yet include custom element specifics in non-custom-element-declarations, but this should be fairly easy to implement.

TagNameDoc

In order to add a TagNameDoc type to the schema I came up with a data structure of kind "tagName". What do you think of this suggestion (see example)? Or do you have a better suggestion?

{
  "version": "experimental",
  "modules": [
    {
      "path": "definition.ts",
      "exports": [
          {
              "kind": "tagName",
              "name": "text-field",
              "declaration": {
                   "name": "TextField",
                   "module": "declaration.ts"
               }
          }
       ]
     },
     {
        "path": "declaration.ts",
        "exports": [
           {
               "kind": "class",
               "description": "A text field custom element",
               "name": "TextField",
               "attributes": [
                  {
                      "name": "size",
                      "defaultValue": "\"large\"",
                      "type": "\"small\"|\"large\""
                 }
             ]
         }
   ]
}

Questions

Here are some more questions/considerations I wrote down while implementing the schema:

  1. How would this schema handle custom elements built with a non-class based approach? For example, how would the output look if I was to use the haunted library? Would we just put all members into one single CustomElementDoc and don't output inheritance in that case?

  2. Mixins would end up being included twice in the export array for a given module: (1) both as a FunctionDoc but (2) also as MixinDoc. Are we fine with mixins being included twice, or should I only include a given symbol as a FunctionDoc if it's not a MixinDoc?

  3. Right now mixins have kind: "class" even though the exported symbol is actually a function. Should we introduce a kind: "mixin"?

  4. I have come across this pattern a couple of times when building support for mixins: class MyElement extends MyMixin(MyBase1, MyBase2), which basically opens up for having multiple superclasses. This schema, doesn't support having multiple superclasses, so do we want to support this pattern, or is it an edge case? Right now, WCA would just set the superclass for MyElement as a reference to MyBase1, and MyBase2 would end up in the exports array but without a reference.

  5. I'm a bit in doubt of how I should implement summary field. I understand that it's a markdown summary suitable for display in a listing, but I think I would need a concrete example of how it could be used, and what I should be outputting for this field :-)

This was referenced Sep 25, 2020
* TODO: restrictions on markdown/markup. ie, no headings, only inline
* formatting?
*/
summary?: string;
Copy link
Collaborator

@thepassle thepassle Sep 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default?: string;

Is present in WCA output, but not in the schema

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@justinfagnani
Copy link
Collaborator Author

I just pushed a big commit where I cleaned up a lot of names and pulled out some shared interfaces in order to make things more consistent.

Changes:

  • Removed the Doc suffix. This was done initially because we wanted a Function interface and that clashed with the built-in Function type in TypeScript. So I changed the declarations to have a Declaration suffix: ClassDeclaration, FunctionDeclaration and VariableDeclaration. Other interfaces have simple names like Slot or Attribute.
  • Changed Package#version to Package#schemaVersion to clarify that it's not the npm package version string.
  • Renamed ModuleDoc to JavaScriptModule and added a kind field to allow for non-JS module types in the future.
  • Separated declarations from exports in JavaScriptModule. This allows non-export but still accessible classes (like superclasses or helpers) to be documented.
  • Added JavaScriptExport which contains an export name and a reference that's exported. This allows for renames, and re-exports.
  • Added CustomElementExport which represented a global customElements.define() call. (I think scoped registries will have to have a different approach)
  • Added parts and cssProperties to CustomElement which were just missing
  • Added a Type interface which contains a string representation of a type as well as a number of references indexed to the type string. This allows for cross-referenceable type annotations without consuming tools having to know anything about the type system. See How to handle types #6 (comment)
  • Added a summary field to every interface with a description
  • Extracted most of ClassDeclaration into ClassLike so that it can be shared with Mixin and allow Mixin to have it's own kind value.
  • Added PropertyLike as the common interface for variables, class fields, and function parameters. PropertyLike adds type and default to those.
  • Added inheritedFrom?: Reference to ClassField and ClassMethod to allow for "flattened" class members with references to the class that added them to the prototype, and match WCA. See Initial schema #9 (comment)
  • Made a named Parameter interface for function parameters and dded optional?: boolean.
  • Added a license and formatting

@runem this is a lot of changes here in names, but doesn't change the overall structure much outside of declarations / exports. WDYT?

@justinfagnani
Copy link
Collaborator Author

@thepassle

If only all declarations that are reachable from exports should be in this doc, we still wont be able to get the class information on MyElement in the following example:

class MyElement extends HTMLElement {
  foo = "bar";
}
customElements.define('my-element', MyElement);

I was thinking declarations should contain all declarations for a module

I would consider MyElement reachable from an export in this case.

That guidance is there because we don't want packages documenting private declarations that are never reachable from outside their module. That's just noise. But there are often things that aren't directly exported that are still reachable and need to be documented. We can adjust phrasing on the comments.

@justinfagnani
Copy link
Collaborator Author

CustomElementDefinition vs CustomElementExport: I'm open to either. It's nice to have consistency in that everything in the exports array is named *Export.

@thepassle
Copy link
Collaborator

thepassle commented Sep 28, 2020

CustomElementDefinition vs CustomElementExport: I'm open to either. It's nice to have consistency in that everything in the exports array is named *Export.

I see what you meant with CustomElementExport now, and it makes sense 👍

Something else:
Should a module have an imports array for third party modules? It could allow tools to display dependencies for a given component (e.g.: MyElement depends on lit-element)

Example:

export interface Import {
  kind: "named" | "default" | "import",
  specifier: string,
  identifiers: Array<Identifier>
}

export interface Identifier {
  name: string,
  alias?: string,
}
import { LitElement } from 'lit-element';
import { css as style } from 'lit-element';
import html from 'lit-html';
import 'some-module';

class MyElement extends LitElement {
  static styles = style``;
  render() { return html`` }
}

// custom-elements.json

module: {
  imports: [
    {
      kind: "named",
      specifier: "lit-element",
      identifiers: [
        {
          name: "Litelement"
        },
        {
          name: "css",
          alias: "style"
        },
      ]
    },
    {
      kind: "default",
      specifier: "lit-html",
      identifiers: [
        {
          name: "html"
        }
      ]
    },
    {
      kind: "import", // better name for this kind of import?
      specifier: "some-module",
    },
  ],
  exports: [],
}

Or would this only add clutter? Arguably you could also just pick a packages dependencies from its package.json. The danger with adding more stuff is that at some point we're just gonna become another AST implementation...

@justinfagnani
Copy link
Collaborator Author

@thepassle

I think ideally imports are an implementation detail. A package should be able to change around its dependencies and internal module structure without a public-facing change.

I would want to start from the use case here. Presumably catalogs are going to want to label and filter components based on dependencies. You can do some of this via package.json, but if might be better to just allow some kind of labeling right in the schema and let tools filter that way.

Github and npm tags are ok for this, though they're often stuffed with values to game search. Maybe we could do a key:value pair in a metadata field and tools can support well-known keys like library?

@thepassle
Copy link
Collaborator

thepassle commented Oct 1, 2020

Considering the following usecase:

const NestedMixin = superclass => class NestedMixin extends NestedA(NestedB(superclass)) {}

If a class uses the NestedMixin, there currently isnt a good way for the class to know that NestedMixin also consists of NestedA and NestedB.

A solution for this could be to allow a @mixin jsdoc tag on VariableDeclaration and FunctionDeclarations, like so:

import {NestedA, NestedB} from 'foo';

/**
 * @mixin {NestedA}
 * @mixin {NestedB}
 */
export const NestedMixins = superclass => class NestedMixins extends NestedA(NestedB(superclass)) {}
import {NestedA, NestedB} from 'foo';

/**
 * @mixin {NestedA}
 * @mixin {NestedB}
 */
export function NestedMixins(superclass) {
  return superclass => class NestedMixins extends NestedA(NestedB(superclass)) {}
}

And add a mixins?: Array<Reference> field to VariableDeclaration and FunctionDeclaration:

export interface VariableDeclaration extends PropertyLike {
  kind: 'variable';
  mixins?: Array<Reference>
}

export interface FunctionDeclaration extends FunctionLike {
  kind: 'function';
  mixins?: Array<Reference>
}

Real life usecase:
https://github.com/ing-bank/lion/blob/8d8867cb1d8a893e42db51446a39c713f32f3fc2/packages/core/src/DisabledWithTabIndexMixin.js#L14

@justinfagnani
Copy link
Collaborator Author

@thepassle Polymer Analyzer handled this with three annotations: @mixinFunction, @mixinClass and @appliesMixin: https://polymer-library.polymer-project.org/3.0/docs/tools/documentation#class-mixins Mixins themselves can use the @appliesMixin for nested mixins.

Polymer Analyzer looked at a pair of @mixinFunction annotated function and @mixinClass annotated class expression as a single mixin entity.

I'm trying to represent that here with a MixinDeclaration interface that extends from both ClassLike and FunctionLike. It'll have a mixins field inherited from ClassLike, and arguments/return inherited from FunctionLike.

export interface MixinDeclaration extends ClassLike, FunctionLike {
  kind: 'mixin';
}

@thepassle
Copy link
Collaborator

Wow, I had totally missed that 🤯 Sorry bout that. That should cover the usecase I mentioned 👍

@thepassle
Copy link
Collaborator

thepassle commented Oct 22, 2020

@justinfagnani Suggestion on how to move forward:

  • I think we should merge this
    I found a few quirks while implementing @custom-elements-json/analyzer and @custom-elements-json/helpers, but I think we ironed them out, and its in good enough state to communicate the work in this repo(/the schema) to more people
  • Itd be nice to get this usecase merged as well Testing use case #12
  • I would like to make a demo repo with some vanilla custom-elements, and a custom-elements.json made by @custom-elements-json/analyzer, with some information/documentation on custom-elements.json, a link to the actual schema (here) that we can then share with the broader community again, like serhii (api-viewer), but also groups like Stencil, to see what their thoughts are, and then move forward from there?

I think having an example or demo repo/custom-elements.json may be easier to grok than just looking at the schema alone

What do you think of that plan?

@adamdbradley
Copy link

adamdbradley commented Oct 23, 2020

This is great to standardize and looking forward to building tools around what's developed here. If there's any data I can help provide is that this json file is generated by parsing all Ionic components: https://unpkg.com/browse/@ionic/core@5.4.1/dist/docs.json

As you can see it has the same goals here, and I'm not proposing it uses this format, but rather it might be helpful to see what data is useful to generate Ionic's docs site.

With this json file we're able to auto generate the each component's docs, such as: https://ionicframework.com/docs/api/button

The button's API was actually pulled from the source:
https://github.com/ionic-team/ionic-framework/blob/master/core/src/components/button/button.tsx

And we also found it useful to use the readme.md file for the free-form documentation: https://github.com/ionic-team/ionic-framework/blob/master/core/src/components/button/readme.md while the JSDocs in the components generated the actual API.

The goal here is also similar to Capacitor's docgen for capacitor plugins: https://www.npmjs.com/package/@capacitor/docgen

One thing, that I might be overlooking, is a string summarizing a method's signature, like myMethod(arg: string) => Promise<number>. While this could get generated from each tool's output, having this pre-build makes it pretty simple for tooling to loop through and print out. Here's some of the types we found to be useful to auto generate docs for a TS capacitor plugin: https://github.com/ionic-team/capacitor-docgen/blob/main/src/types.ts#L29

@thepassle
Copy link
Collaborator

thepassle commented Oct 23, 2020

Thanks for sharing @adamdbradley ! I'll go over it today and try to find the similarities and where the formats diverge, and what we can improve on. We really want to make sure custom-elements.json fits all required usecases, so it can be usable by the broad WC ecosystem

One thing, that I might be overlooking, is a string summarizing a method's signature, like myMethod(arg: string) => Promise

This is a fair point, we do have all the information available to construct the method signature under:
method.name, method.parameters and method.return.type, so it should be fairly straightforward for tools to construct the signature 🙂

@justinfagnani
Copy link
Collaborator Author

Yes, thanks @adamdbradley for the links! I'll look at them too.

Regarding method signature, I think I'd rather keep the data normalized and spin up helper libraries to stringify declarations. One reason is that we can offer a choice of to-string functions that can syntax highlight and hyperlink the signatures. I'd love it if it became expected practice in WC docs that they were cross-referenced internally, to other libraries, and MDN.

Speaking of, I want to make sure we have optional references in all the schema types that need them for that to work.

@justinfagnani
Copy link
Collaborator Author

@thepassle I'm ok with merging this now and iterating. We have verbal +1s from the rest of the open-wc team, and you've been the most involved in the review here. I want to do another quick pass and set a pre-release shchema version so it's more clear that this isn't the final form. Then we can file issues and PRs for the rest.

The only thing that would make me hesitate on actually publishing this is the name: #22 I'm just a little wary of naming the spec after the presumed filename. Thoughts (in that issue)?

@thepassle
Copy link
Collaborator

Regarding method signature, I think I'd rather keep the data normalized and spin up helper libraries to stringify declarations.

This could easily be implemented in https://github.com/open-wc/custom-elements-json/tree/master/packages/custom-elements-json-helpers 👍 (warning: very very unfinished, but now that eslint-plugin-lit-a11y is released, I'll be able to spend some more time on both the analyzer and helpers library)

The only thing that would make me hesitate on actually publishing this is the name: #22 I'm just a little wary of naming the spec after the presumed filename. Thoughts (in that issue)?

I'll take a look

@thepassle
Copy link
Collaborator

Just took a closer look at your button example @adamdbradley
(these:
https://github.com/ionic-team/ionic-framework/blob/master/core/src/components/button/button.tsx
https://ionicframework.com/docs/api/button)

As far as I can see, all the info you guys have in there is currently supported by custom-elements.json 🙂

The only thing I couldnt really figure out is this:
For example in the expand attribute, you have the following description: Full-width button with rounded corners.

I dont see that description in the source code, do you somehow add it later? 🤔

The only other thing I can think of thats different is indeed method signatures, but as discussed those could be easily implemented by helper libraries

@justinfagnani
Copy link
Collaborator Author

Thanks for the reviews everyone! Since it seems like there's consensus on this general shape of schema, I'm going to merge now and continue iteration in follow-on issues and PRs.

@justinfagnani justinfagnani changed the title Initial attempt at a schema Initial schema Oct 26, 2020
@justinfagnani justinfagnani merged commit 0e68638 into master Oct 26, 2020
@justinfagnani justinfagnani deleted the schema-1 branch October 26, 2020 18:52
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.

None yet

9 participants