Closed runem closed 4 years ago
Hey @runem ;-)
Great summary :tada: :clap:
My feedbacks as a nested list:
@mixes, @example, @method and @typedef
About JavaScript examples (sorry I can't judge TS):
Example 1:
The result table seems misaligned. I was expecting this:
Prop Name | Attr Name | Type | Description | Default | Required | Visibility |
---|---|---|---|---|---|---|
foo | "red"|"green" | Test 1 | "green" |
|
for the markdown table :wink:Example 2:
MyType
!!HTMLELement
, is it supposed to pick up static get properties ()
from LitElement?Example 3:
HTMLELement
, is it supposed to pick up static get properties ()
from LitElement?red
in the example."bar 2"
Thank you so much for your feedback! :-)
I've now fixed the problem in the example code :+1:
Add 'public' if visibility is not provided? I think this depends on the transformer transforming the output. I left the fields empty in the tables to indicate that they are "undefined" in the output, but the markdown/json transformer could "force" public if we want to.
Below, I tried to output what WCA currently outputs using the markdown format. As you see, it only shows the "Visibility" column if one of the rows contain a non-public member. I think this works pretty well :-)
Add 'no' if required is false? The markdown formatter will add required in the 'Default' column when member is required.
Pick up static get properties ()
when class doesn't extend LitElement?
At the moment the analyzer picks up static get properties ()
even though the class doesn't extend LitElement. I made it this way because I'm not entirely sure if I can follow all inheritance trees (eg. edge case mixins that the parser doesn't understand).
Below is actual markdown output from the analyzer on the refactor
branch
Property | Type | Default | Description |
---|---|---|---|
foo |
"red"\|"green" |
"green" | Test 1 |
Property | Visibility | Type | Default | Description |
---|---|---|---|---|
foo |
protected | MyType |
{} | Test 1 |
Attribute |
---|
foo |
Property | Attribute | Visibility | Type | Default | Description |
---|---|---|---|---|---|
foo |
bar |
private | ("blue" \| "red) |
"bar 2" | Test 1 |
I think this depends on the transformer transforming the output. I left the fields empty in the tables to indicate that they are "undefined" in the output, but the markdown/json transformer could "force" public if we want to.
Makes sens :+1:
The markdown formatter will add required in the 'Default' column when member is required.
OK
At the moment the analyzer picks up static get properties () even though the class doesn't extend LitElement. I made it this way because I'm not entirely sure if I can follow all inheritance trees (eg. edge case mixins that the parser doesn't understand).
I totally understand how it could be difficult. I guess users will probably rarely have this edge case. It's not worth it to invest in this.
Perfect :+1: Great work @runem :clap: :clap: :clap: :clap:
I'm ready to try this with my components (I'm also looking forward for the fix about ternary operator) :wink:
I just released v0.1.21
that includes the ternary operator fix :+1:
I also released a beta version of the above :tada: It would be really helpful to me if you would like to test it on the component that you were having trouble with in https://github.com/runem/web-component-analyzer/issues/124 - thank you so much for you help so far :-)
You can test the beta version by running this:
npx web-component-analyzer@1.0.0-next.1 analyze my-element.js
I saw the 0.1.21
so I directly started testing it and it works great!!!
But hey, I missed this message about 1.0.0-next.1
so now I know what I'm about to try ;-)
See you in a few minutes/hours...
I'll directly try the 1.0.0-next.9
;-)
With 1.0.0-next.9
static get properties
:+1:render()
, firstUpdated()
are always picked up in the list of methods@private
for now.Questions:
CustomEvent.detail
property.@mixes
yet?Sorry if my questions look too impatient :smile: this is great work :+1: :clap:
With this:
/**
* Some docs
*
* @attr {string|number} datetime - Date as ISO string or timestamp
*/
export class CcDatetimeRelative extends HTMLElement {
static get observedAttributes () {
return ['datetime'];
}
get datetime () {
return this.getAttribute('datetime');
}
set datetime (value) {
this.setAttribute('datetime', value);
}
I get this:
| Property | Attribute | Type | Description |
|------------|------------|------------------|---------------------------------|
| `datetime` | `datetime` | `string \| null` | Date as ISO string or timestamp |
|
inside backticks.string|number
. When I remove the getter/setter, I get string|null
. Should I add required to remove the null?BTW: I tried the color type ;-) Works great in JSON but I guess the markdown template does not use it yet.
Thank you so much for your feedback, I really appreciate it! :hugs:
string | null:
Great catch with! This was a problem where i forgot to merge the typeHint
when merging an attributes with property. It should be fixed with 1.0.0-next.11
:+1:
LitElement-specific methods:
I'm already excluding HTMLElement/LitElement-specific methods by default. Are you including/patching your own version of LitElement, or are you using the library directly? I tried to change the exclusion-check to check for more cases of LitElement in 1.0.0-next.11
, so that version might work for you :+1:
Document event type:
If you are using Typescript you can use the generic detail like: new CustomEvent<number>({detail: 123})
, however I'm not sure at the moment how to document events, because you can also dispatch non-custom-events (without the detail), and In that case, it wouldn't be correct to always assume that the type
is the type of detail. Right now I actually support this JSDoc syntax:
/**
* @fires {string} change - This is a change event
*/
and
/**
* @type {string}
*/
this.dispatchEvent(new CustomEvent("my-event"));
(with 1.0.0-next.11
)
This will however assume the type "string" to be the detail
type which I'm not sure is the best solution (because of the above) :blush:
Include type in CSS property markdown:
I forgot to include it! That's included in 1.0.0-next.11
:+1:
@mixes
jsdoc:
I'll take a look later today to see if it's a low hanging fruit :blush:
@hsablonniere I'm not that experienced using the @mixes
jsdoc, - do you have an example that I can test while developing the functionality? :sunny:
OK, so I tried with 1.0.0-next.11
:
string | null
:
String|Number
is fixed :+1:\
before the |
LitElement-specific methods:
render()
method.custom-elements.json
thread
Document event type:
{CustomEvent<MyType>}
, I love the idea and it works well with 1.0.0-next.11
.detail
to be MyType
in a good way.{String}
for an event should not imply it's a CustomEvent
with detail
as a String
. It's too implicit and does not handle cases where you dispatch a non CustomEvent
.@fires
from JSDoc.Include type in CSS property markdown:
@mixes
jsdoc:
Ah ah, I was expecting feedbacks from you, I don't really know what I'm doing with this :p
|
in markdown:
I included the backslash for |
because else it's interpreted as a the markdown table character |
and the table will be malformed. If you include it in github markdown it will look fine :blush: Do you have problems in other markdown parsers where it doesn't work?
LitElement-sepcific methods:
Aha! The render
method found in cc-toggle.js
is not properly excluded. In 1.0.0-next.12
it should be fixed, and LitElement-specific methods will be interpreted as protected
. However, I'm considering making it possible to exclude/include these methods using CLI options instead of forcing these methods to be protected
:+1:
@mixes
jsdoc:
I will open a seperate issue on this to get some feedback
I think Vaadin has similar use cases.
We use @mixes
in our code, as pointed above.
BTW I'm planning to check the new version tomorrow using these branches:
https://github.com/vaadin/vaadin-accordion/tree/next https://github.com/vaadin/vaadin-details/tree/next
| in markdown:
I guess there is not solution for this because:
I say, let's keep the current behaviour, I have a plan to move away from this Storybook addon and use the JSON output at some point.
LitElement-sepcific methods:
@mixes
jsdoc:
Great!!
@web-padawan I knew you were not that far ;-)
@runem I'm using the exact same pattern as what @web-padawan is doing with https://github.com/vaadin/vaadin-element-mixin/blob/master/vaadin-element-mixin.html
The mixin:
export function withMyMixin (ParentClass) {
return class extends ParentClass {
static mixinMethodFoo() {}
mixinMethodBar() {}
}
}
Using the mixin:
export class MyComponent extends withMyMixin(HTMLElement) {
// ...
}
I guess the only differences are naming conventions and case sensitive code formatting ;-)
Sorry, we should wait for a thread for this, I'll copy/paste the example...
I just tried 1.0.0-next.13
.
@example
yet but I tested it and it works well :+1:About methods, as you know I don't use TypeScript and I was wondering how far we could go with JSDoc to document function object/array types...
This is a method on my map component:
/**
* This description works well and is picked up by WCA
* @param {Point[]} points - This description is ignored but I'm OK with it
* @param {PointsOptions} options - This description is ignored but I'm OK with it
*/
addPoints (points, options = {}) {}
The thing is that I get any
in the markdown output:
| Method | Type | Description |
|-------------|------------------------------------------|-----------------------------------------------------|
| `addPoints` | `(points: any[], options?: any) => void` | This description works well and is picked up by WCA |
@hsablonniere I support both your mixin pattern and what can be found in https://github.com/vaadin/vaadin-accordion :+1:
@web-padawan I'm very interested in hearing if everything works for you. I tested it on https://github.com/vaadin/vaadin-accordion and chose to implement support for readonly
properties such as getters likes get items(): VaadinAccordionPanel[]
. It's included in 1.0.0-next.14
:sunny:
I also see that the DetailsStateMixin
in https://github.com/vaadin/vaadin-details looks like this, but ControlStateMixin
is not picked up, because Base
is not being resolved correctly by WCA. I'm on it :+1: (update: the pattern is supported in 1.0.0-next.15
)
export const DetailsStateMixin = <T extends VaadinDetailsBase>(base: T): DetailsControlState => {
const Base = ControlStateMixin(base as VaadinDetailsBase);
class VaadinDetailsStateMixin extends Base
@hsablonniere Documenting primitive types like @param {number} points
should work, but I will take a look at supporting the @param
jsdoc tag :-)
Also, if I were to output param descriptions, do you have an idea for how to include it in the markdown? Here are two suggestions:
Method | Type | Description |
---|---|---|
addPoints |
(points: any[], options?: any) => void |
This description works well and is picked up by WCA points: Description of first parameter options: Description of second parameter |
Method | Type | Description | Parameters |
---|---|---|---|
addPoints |
(points: any[], options?: any) => void |
This description works well and is picked up by WCA | points: Description of first parameter options: Description of second parameter |
@runem About function params, I would be OK with not having the param description at all but I think I prefer the first example with one description block and a list of params in the same block.
Also, I will try the 1.0.0-next.15
because I think I have one use case of @readonly
.
I was not able to make @readonly
work with JSDoc. Tried on the static get properties
and the setter.
Small bug I noticed in the last component I'm updating.
With this small test case:
export class MyComponent extends LitElement {
firstUpdated () {
myChartLib.init({
fooMethod() {
// Hello
},
barMethod: () => {
// Hello
}
})
}
}
The method fooMethod
is picked up:
## Methods
| Method | Type |
|-------------|--------------|
| `fooMethod` | `() => void` |
I had an example where I used this method pattern in a config object for charts.js and the method was picked up by WCA. As you can see, when I use an arrow function, it solves the problem.
Sorry if I pollute this thread too much but I need to thank you @runem for all your efforts, I just finished refining our docs and it's awesome... :+1:
I used to have a broken list of props, I used to manually write a markdown table to fix this. Many other small details are improved now!!! :clap:
Example before:
Example after:
In case you're interested in how your tool improved our codebase, here's the commit:
My next steps:
@hsablonniere It looks great! I'm really happy so see it working that well!! :tada:
Whoops, great catch! This is because I don't check that the method is a member of the class :+1: Fixed in 1.0.0-next.16
@readonly
jsdocSorry, forgot to mention that this only works for properties at the moment :-)
I just finished improving support for methods/jsdoc in1.0.0-next.16
. It was quite challenging to get this working, but I managed to solve it! It hope it works for you :+1:
Now this code:
/**
* This description works well and is picked up by WCA
* @param {Point[]} points - This description is ignored but I'm OK with it
* @param {PointsOptions} options - This description is ignored but I'm OK with it
*/
addPoints (points, options = {}) {}
Will result in this table:
Method | Type | Description |
---|---|---|
addPoints |
(points: Point[], options?: PointsOptions): void |
This description works well and is picked up by WCA points: This description is ignored but I'm OK with it options: This description is ignored but I'm OK with it |
I will soon release 1.0.0
so if we don't find any more problems during the next couple of days I plan on releasing :tada:
It works, thanks very much @runem :+1: :clap:
Is the methods data also included to JSON output? I would like to use it for api-viewer
.
Same question about the mixins.
@web-padawan No, these are not included yet. We still haven't agreed on an optimal format for documenting methods
and mixins
, but I will include them in the format as soon as we have some more agreement on https://github.com/webcomponents/custom-elements-json/ :-)
@runem another question about the mixins: even though they are not included, can we at least get the properties implemented in mixins documented on the custom element?
E.g. I have a separate mixin for handling disabled
property, etc.
@web-padawan No, these are not included yet. We still haven't agreed on an optimal format for documenting methods and mixins, but I will include them in the format as soon as we have some more agreement on https://github.com/webcomponents/custom-elements-json/ :-)
I guess it makes sense but I think we should consider having custom extensions to the format. In OpenAPI, you can specify properties with "x-somethin"
.
I may want to document the i18n keys used by my component for example.
@web-padawan properties found on mixins are documented on the custom element, so if you don't see the disabled
property declared in a mixin, it's because WCA doesn't pick up on your mixin. I'm working on supporting as many mixin-patterns as possible, so I'm very interessted in seeing the code that doesn't for work you :-)
Be aware that currently there is one big limitation in WCA which I will work on addressing after v1.0.0. The limitation is that when consuming libraries shipped without Typescript definition files, the library code is not analyzed at all (for example using a mixin that is declared in a library). This is due to how the Typescript-parser works. There is, however, a solution to this :+1:
In addition, everything seems to be working fine when I try WCA on for example the next
branch of vaadin-details
(please correct me if I'm wrong). Here it correctly follows the mixin DetailsStateMixin
to ControlStateMixin
which is found in this declaration file from node_modules:
./node_modules/@vaadin/control-state-mixin/control-state-mixin.d.ts
export interface ControlStateInterface {
autofocus: boolean;
disabled: boolean;
tabIndex: number | null;
focus(): void;
blur(): void;
}
This is the analyzed inheritance tree:
{ VaadinDetails } <-- ( DetailsStateMixin ) <-------------------------- { DetailsBase }
{ LitElement } <----- { ControlStateInterface } { VaadinElement }
{ UpdatingElement } { VaadinElement_base }
{ HTMLElement }
Below follows the entire output which includes properties found on mixins
<vaadin-details>
is a Web Component creating an expandable panel
similar to <details>
HTML element.
Mixins: DetailsStateMixin, ControlStateMixin
Attribute | Type | Description |
---|---|---|
focus-ring |
Boolean |
Set when the element is focused using the keyboard. |
focused |
Boolean |
Set when the element is focused. |
Property | Attribute | Type | Default | Description |
---|---|---|---|---|
autofocus |
boolean |
|||
disabled |
boolean |
|||
opened |
opened |
boolean |
false | When true, the panel content is expanded and visible |
tabIndex |
number \| null |
Method | Type |
---|---|
blur |
(): void |
focus |
(): void |
Event | Description |
---|---|
opened-changed |
Fired when the opened property changes. |
Name | Description |
---|---|
Slot fot the collapsible details content. | |
summary |
Slot for the summary content. |
Part | Description |
---|---|
content |
The wrapper for the collapsible details content. |
summary |
The element used to open and close collapsible content. |
summary-content |
The wrapper for the slotted summary content. |
toggle |
The element used as indicator, can represent an icon. |
@runem thanks, will try how it works for us.
Version 1.0.0
is out now :tada: I'm really grateful for all of your help! It has been very, very valuable to me 🤗
I will close this issue now, but I'm creating some follow up issue for [discussion] @mixins jsdoc
and support dependencies without TS-typings
.
I have been spending the last couple of weeks on refactoring/improving many parts of the analyzer. Most notable, I have been improving performance, jsdoc-support and member-merging. Some of the requirements meant that I had to change core parts of the analyzer, eg. to support lazy-evaluation for performance improvements. Soon I'll be finishing up the work, push it to the branch
refactor
, and afterwards I will begin preparing a version1.0.0
release.The following is a description of what have changed. Feel free to give your feedback, thoughts or additional needs. It would be especially interesting to get some feedback on my examples of how component members are merged now.
Performance
In the past, the entire component, with all of its features, was analyzed when running the analyzer. This means, that an IDE-plugin discovering custom element tag names (eg. on startup) had to analyze all elements, which could result in performance issues. The analyzed components would often be of no use, because only a subset of components are actually used in a given source file. In order to address performance issues, I'm implementing the following 4 things in WCA:
Library exports
In order to use WCA in the browser such as https://runem.github.io/web-component-analyzer, I will make some changes to what this library exports. Specifically, I will:
Diagnostics and metadata
In the past, WCA was able to emit both documentation and diagnostics. In order to clean up things, I'm removing the functionality to emit diagnostics. Instead, it's now possible for WCA to add flavor-specific metadata to analyzed features that can be read programmatically. For example, the content of LitElement's
@property
decorator will now be available to read on the result. Thuslit-analyzer
will now be able to do more things with LitElement-specific information, eg. to emit diagnostics.JSDoc
I will look into supporting
@extends
,@mixes
,@example
,@method / @function
,@typedef
. I haven't started working on them, but the architecture supports it now :-) Existing supported jsdoc tags will work better because of improved merging logic.The architecture
I have been making a lot of changes to the way components are analyzed. It's now possible for flavors to hook into much more than before in a much cleaner flow. This image should give an overview of the architecture:
Analyzing and Merging
Each flavor finds features on components. Features can be "properties", "attributes", "slot", eg. Multiple features can be emitted per property (eg. if you have both a constructor assignment and a class field that reference the same property). I'm making a lot of changes to how these features are merged.
Here are some of the highlights:
required
) prefers the value of the first non-undefined value found@type
JSDoc. In JS-file the@type
JSDoc is preferred over the type checkerHere are some examples so you can see how merging will work. They are a bit contrived, but that's on purpose because I want to show how overlapping fields and comments are merged.
Examples using Javascript
Example 1
Result
Example 2
Result
Example 3
Result
Examples using Typescript
Example 1
Result
Example 2
Result
Example 3
Result