emberjs / rfcs

RFCs for changes to Ember
https://rfcs.emberjs.com/
791 stars 408 forks source link

Documenting Components (arguments, attributes and yield) #591

Closed gossi closed 2 years ago

gossi commented 4 years ago

I'm looking for a convention/standard to document components, especially targeting these information:

for components, template-only components while supporting for java- and typescript. Primarily focused for glimmer components, yet the js-only approach should also be suitable to be applied to ember components, too.

The idea is, that these conventions can be exported into an intermediate format, which itself can be consumed by documentation tools (such as ember-cli-addon-docs or storybook) and on the other hand for tools providing support at our fingertips, e.g. (unstable) ember language server, so we have suggestions through intelli-sense/auto-completion in our preferred editors.

Arguments

Arguments can easily be described as an interface as this already the proposed way for doing this in glimmer components, when using typescript:

export interface InputGroupArgs {
  value: string;
  name: string;
  label?: string;
  error?: string;
  changed?: (value: string) => void;
}

/**
 * An input group is an input field with additional label and error field
 *
 * Usage:
 * 
 * ```hbs
 * <InputGroup @value="abc" @name="given-name" @changed={{this.updateGivenName}} />
 * ```
 */
export default class InputGroupComponent extends Component<InputGroupArgs> {}

JS / Template-only Components

For javascript and template-only components I propose the same approach as above in a component.d.ts file. Along with that is a side-benefit on top of it, which is type-coverage of your package. Second is, that most editors already use it as input to provide intelli-sense mechanism, even if the choice is to use javascript over typescript.

Alternative

Instead of using a component.d.ts regular JS can be used and special @argument tags in docblock can be used (as it is done today with ember-cli-addon-docs):

/**
 * An input group is an input field with additional label and error field
 *
 * Usage:
 * 
 * ```hbs
 * <InputGroup @value="abc" @name="given-name" @changed={{this.updateGivenName}} />
 * ```
 * 
 * @argument string value
 * @argument string name
 * @argument (string) label
 * @argument (string) error
 * @argument (Function) changed
 */
export default class InputGroupComponent extends Component {}

Drawback of this approach is, that would only work for JS components and is not a solution for template-only components. Second is, that supported types are usually limited to what jsdoc et al. support and not as rich as in comparison to the typesystem typescript offers.

Yield

The type for yield is an array and many people use the first entry to contain an object/hash. First separate between how to document the yield type itself and second where to make the connection between the typed yield and the component.

Typing yield:

import InputGroupLabelComponent from '...';
import InputGroupErrorComponent from '...';

interface InputGroupBuilder {
  Label: InputGroupLabelComponent;
  Label: InputGroupErrorComponent;
}

/**
 * Whether the input group has an error
 */
type IsErrored = boolean;

type InputGroupYield = [
  InputGroupBuilder,
  IsErrored
];

For connecting yield to the component I see two options:

Option 1:

Using a special jsdoc tag @yield (as been done in ember-cli-addon-docs today):

/**
 * [...]
 * 
 * @yield InputGroupYield
 */
export default class InputGroupComponent extends Component<InputGroupArgs> {}

Option 2:

Extend generics to accept a second value as yield:

/**
 * [...]
 */
export default class InputGroupComponent extends Component<InputGroupArgs, InputGroupYield> {}

There is no real benefit from a technical perspective as you are not accessing the yield block from the code as of today and I don't see that happening in the future. Instead would block a slot of the generics, that might be better suited to something else in the future.

As a counter to this, the pair reads coherently as ... extends Component<Input, Output> (which is why I personally like it).

Attributes

There is already an issue open regarding this: #586 (...attributes -- Component API and Discoverability). In speaks about a list of things unknown to the consumer of a component and where passed attributes will end up (if ever). Whereas the argument is that those information are implementation details of the respective component to decide on that.

While I agree this is implementation details I as a consumer still better have some information what I can do use attributes for. It still belongs to the components internals but here are my questions I have when passing down attributes:

That's mostly a11y related. Given the element + attribute combination has a massive effect on listener usage: e.g. whether I can use a {{on 'click' ...}} modifier or not.

!!!

Provide Suggestions

!!!

Appendix

You may want to document your component.ts (typescript) or component.d.ts (js / template-only) as described above. Using the generics option for yields, this is how the full example looks like:

import Component from '@glimmer/component';
import InputGroupLabelComponent from '...';
import InputGroupErrorComponent from '...';

interface InputGroupBuilder {
  Label: InputGroupLabelComponent;
  Label: InputGroupErrorComponent;
}

/**
 * Whether the input group has an error
 */
type IsErrored = boolean;

type InputGroupYield = [
  InputGroupBuilder,
  IsErrored
];

export interface InputGroupArgs {
  value: string;
  name: string;
  label?: string;
  error?: string;
  changed?: (value: string) => void;
}

/**
 * An input group is an input control with additional label and error fields
 *
 * Usage:
 * 
 * ```hbs
 * <InputGroup @value="abc" @name="given-name" @changed={{this.updateGivenName}} />
 * ```
 */
export default class InputGroupComponent extends Component<InputGroupArgs, InputGroupYield> {}

Let's discuss to fill the blank spots.

pzuraq commented 4 years ago

I've been thinking about this as well recently, and I like the line of thinking that we should be using Typescript to standardize docs. api-extractor has been advancing pretty quickly, and it can document anything that TS understands (including plain JS), so I think it would work pretty well here.

Re: TO-Components, can we potentially declare an TO-component interface that matches Glimmer component's generics, such that you can add a .d.ts file that does something like this?

declare class MyTOComponent extends TemplateOnly<Args, Yields> {}

Re: Yields/blocks, I think that extending generics makes total sense, but I think the typing probably wants to be a bit different for two reasons:

  1. Named yields/blocks are right around the corner, so we need to make sure we account for those

  2. Blocks are conceptually thought of and taught as callbacks, so I think typing them that way would probably be ideal.

import Component from '@glimmer/component';
import InputGroupLabelComponent from '...';
import InputGroupErrorComponent from '...';

interface InputGroupBuilder {
  Label: InputGroupLabelComponent;
  Label: InputGroupErrorComponent;
}

export interface InputGroupBlocks {
  default(builder: InputGroupBuilder, isErrored: boolean): Template;
}

export interface InputGroupArgs {
  value: string;
  name: string;
  label?: string;
  error?: string;
  changed?: (value: string) => void;
}

/**
 * An input group is an input control with additional label and error fields
 *
 * Usage:
 * 
 * ```hbs
 * <InputGroup @value="abc" @name="given-name" @changed={{this.updateGivenName}} />
 * ```
 */
export default class InputGroupComponent extends Component<InputGroupArgs, InputGroupBlocks> {}

Maybe this type could be an override too, such that if it receives a function directly it assumes it is the default block.

MichalBryxi commented 4 years ago

Just got back from the adventures of trying to make clear the difference between arguments and attributes in doc for built-in-components. And there's a few lions living over there. Even the official documentation for <Input /> has to have this complicated section on difference between those two:

In most cases, if you want to pass an attribute to the underlying HTML element, you can pass the attribute directly, just like any other Ember component. ... However, there are a few attributes where you must use the @ version.

My take from this excersice is:

pzuraq commented 4 years ago

I think this is an excellent point, definitely something to think on more.

I will say, I think that in all cases arguments should be preferred over attributes, if they exist and are documented. So, we should prioritize highlighting the various arguments that exist, and if for instance an @ariaLabel arg existed, it should always supercede manually adding aria-label. The assumption there is that the component author had to do something with the argument value dynamically, and expects you to use the arg.

gossi commented 4 years ago

Attributes

Thanks @MichalBryxi - I also like the comments you added in #586

Regarding this @ariaLabel vs aria-label. If the all the component does is aria-label={{@ariaLabel}} the argument shouldn't be there at all and basically looks very much like code smell, as what you are doing is using glimmer components as ember components 🙈 If instead, you have a @label attribute that is used in different locations or you give your internal <label id="...">{{@label}}</label> and then also use aria-labelledby="..." this is a nice usage instead.

That said, in order to understand this, it is good to understand where my ...attributes are added and which attributes will be added by the component. Stating it like that, documenting it sounds very much like over-engineering. What you as a developer might give as information is, what the consumer might be adding on top of it.

Yield/Blocks

@pzuraq and had a discussion on discord where I had mostly questions about understanding the new blocks, so let me summarize how to adress them with documentation:

First: Understand the (new) blocks as a callback you pass into the component, which is why using a generic makes sense here. The simplest use case:

export default class FooComponent extends Component<{}, ((x: string, y: string, z: () => void) => Template)>

and the related template:

<Foo as |x y z|>
    {{x}} {{y}} <button {{on 'click' z}}>z</button>
</Foo>

This is the minimal support as we have them now. The type could also be written as (some might prefer this - including me):

interface FooBlocks {
    default(x: string, y: string, z: () => void): Template;
}

export default class FooComponent extends Component<{}, FooBlocks>

This will set us up for having multiple named blocks:

interface FooBlocks {
    main(x: string, y: string, z: () => void): Template;
    title(): Template;
    footer(): Template;
}

export default class FooComponent extends Component<{}, FooBlocks>

and use it as:

<Foo>
    <:title>I'm atop</:title>
    <:main as |x y z|>
        {{x}} {{y}} <button {{on 'click' z}}>z</button>
    </:main>
    <:footer>I go last</:footer>
</Foo>

I pretty much like the yield/blocks part :]

mehulkar commented 4 years ago

Only one thing: please have a path for non TS users!

pzuraq commented 4 years ago

@mehulkar api-extractor can understand plain JS and document it as well. However, it is limited, in that it can't understand the types of values that are provided, and it can't understand values that don't exist (like blocks for instance).

I think the best short-term solution is to say that users should define a component.d.ts file if they want to add documentation for their component. In the medium-to-long-term, api-extractor does output a plain JSON and Markdown format, so we can expand on that by creating tools that can parse doc comment blocks and add additional metadata to the output.

This is, FWIW, how ember-cli-addon-docs API documenters work, but the process there was ad-hoc and very bespoke. I'm worried that if we try to support all of that up front, it'll end up becoming a much larger project. Focussing on using standard types/tooling first I think is likely the going to be most successful path.

gossi commented 4 years ago

@mehulkar I see *.d.ts a solution for js-only, too. You will still write js only components and for docs you'll add the richness typescript offers (as js alone is too limited here).

jelhan commented 4 years ago

Ember CLI Addon Docs allows to add a description per argument / yield.

See here as an example how this looks like in generated docs: https://ember-learn.github.io/ember-cli-addon-docs/docs/api/components/docs-header

Here is the related documentation written in YUIDoc: https://github.com/ember-learn/ember-cli-addon-docs/blob/master/addon/components/docs-header/component.js#L49

How would this look like with proposed typescript?

I also miss some other features from Ember CLI Addon Docs in this proposal:

I think support to Ember CLI Addon Docs should be added as a first step to compare the available options. And maybe the scope should be reduced to components bundled with Ember itself. At least as a starting point.

pzuraq commented 4 years ago

I think the goal here is to use standard tooling by default. Ember CLI Addon Docs uses a very bespoke, custom system to get that functionality (I wrote it, am very familiar). It's a massive effort to support, and is quite likely to break in the future as more language and framework features are added.

The idea here is to come from the other angle. If all we had were the standard TS/JS tooling options, what would the standard for documentation look like?

From their, we can build on top of the basic standard, and eventually add back a better representation which is more targeted, like the API documentation in ec-addon-docs.

gossi commented 4 years ago

Exactly as what @pzuraq said. Yet there is a valid point to think about alternatives in comparison to (d).ts:

I can think of a converter/codemod that takes already existing ec-addon-docs jsdocs and port its over to whatever might be at some day the standard.

Form what I think we know now the (d).ts way of doing it is the most practical, beneficial one that covers input and output of components. In terms of scope, arguments might be scoped out (or moved to the referenced issue).

pzuraq commented 4 years ago

It also has other benefits, too. If we lean on TS definitions here, in the future it'll mean that future tooling can take advantage of those types, etc. Eventually when we have typed templates, we'll get even more benefits.

jelhan commented 4 years ago

I agree that using TypeScript has a lot of advantages. I'm not against the proposal. I just think that it needs to provide an upgrade path for addons that are using advanced features like descriptions and marking an argument / yield as private or deprecated.

Haven't much experience with it but maybe it's just about naming TSDoc / TypeDoc as an optional addition if more advanced documentation is needed?

pzuraq commented 4 years ago

I think that the main change here would be to expand the type definitions of @glimmer/component, rather than adopting any documenting conventions or tools directly into core. The tooling solutions behind ec-addon-docs and TS are both too volatile at the moment to maintain directly as a core part of Ember, IMO, but for the TS solution to work we’d need to update the types for components.

Whatever docs solutions are built on top of those could build on that base. If ec-addon-docs adds a documenter that understands TS, that’d be great 👍

jelhan commented 4 years ago

I think that the main change here would be to expand the type definitions of @glimmer/component, rather than adopting any documenting conventions or tools directly into core.

Thanks a lot for the clarification. This wasn't clear to me so far. I think it would be helpful to scope a RFC to adding support for TypeScript documentation but not making any recommendation if it should be preferred over other options. Such a recommendation could be added later in a follow-up RFC as soon as we gathered more experience.

lifeart commented 4 years ago

image

and

{{!-- 
    interface Args {
        foo: {
            bar: string
        }
    }
--}}

this is how https://github.com/lifeart/els-addon-typed-templates support template-only components arguments declaration

nightire commented 4 years ago

I'd like to add one more thing to consider that we should teach add-on authors to define a generic component accurately.

I've seen a lot of addons today define their components as:

class InputGroupComponent extends Component<InputGroupArgs> {
  // ...
}

But that's not good enough, because users may need to extend InputGroupComponent to create more suitable components on their own.

A better way could be like this one:

class InputGroupComponent<Args extends InputGroupArgs> extends Component<Args> {
  // ...
}

If we want to document how to define arguments, attributes, and yield, we should consider this as well, so that more and more add-ons can provide a solid foundation for users.

This concern could be a little off-topic at a glance, but it's a real pain point to me ever since I started to use TypeScript in every Ember.js project.

chriskrycho commented 4 years ago

@nightire that's a great callout; I'm quoting it in a new issue to make sure I hit it (along with related points) in the docs refresh for Octane we're slowly working on in the ember-cli-typescript repo!

gossi commented 4 years ago

I was wondering what's the opposite then? Ie. saying "Do NOT extend my component" - use composition instead! Explicitely leaving out this extensibility? Or:

export final class MyComponent .... {}

?

nightire commented 4 years ago

Hmmm...interesting, maybe check the constructor can mimic a final class:

export default class FinalComponent extends Component<Args> {
  constructor() {
    if (this.constructor !== FinalComponent) {
      throw new Error('No no, extending FinalComponent is a bad idea.');
    }
    super();
  }
}
lupestro commented 4 years ago

Just as we are headed toward named blocks after using a single anonymous yield successfully over a number of years, I think we will eventually need to be able to supply splattributes to different named conceptual areas of a component. We may want a conceptual place in documentation for splattributes with an eye toward there being multiple such places in the future.

Concrete Real-world Example: Our QA team depends upon a custom data attribute (rather than ids or classes) to find the places to click and fill in and read text from their selenium tests. I had to write a modifier to apply on the enclosing element to force data attributes into the operational elements of the <PowerSelect> controls we use all over the place so QA could manipulate them during test without having to analyze the structure. (They essentially delegated that analysis to us as we set the attributes in place. We, in turn, would like to be able to delegate it to the team that maintains the component. :) )

NullVoxPopuli commented 2 years ago

@gossi, now that https://github.com/emberjs/rfcs/pull/748 is merged and implemented, should this issue be closed?