Skip to content

Require declared block params rather than inferring them #15

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

Merged
merged 6 commits into from
Sep 6, 2020

Conversation

dfreeman
Copy link
Member

@dfreeman dfreeman commented Sep 6, 2020

Background

There's been an ongoing conversation around this proto-RFC for formalizing component documentation patterns that includes explicitly providing types for the block parameters a component yields.

As things stand in glint prior to this PR, doing so is possible but not particularly ergonomic:

import Component, { hbs } from '@glimmerx/component';
import { AcceptsBlocks } from '@glint/template/-private';

export interface MyComponentArgs<T> {
  values: Array<T>;
}

export interface MyComponentYields<T> {
  default: [value: T, index: number];
  inverse: [];
}

export class MyComponent<T> extends Component<MyComponentArgs<T>> {
  public static template: <T>(
    args: MyComponentArgs<T>
  ) => AcceptsBlocks<MyComponentYields<T>> = hbs`
    {{#each @values as |item index|}}
      {{yield item index}}
    {{else}}
      {{yield to="inverse"}}
    {{/each}}
  `;
}

The only way to accomplish this is by explicitly typing your template value, making use of internal knowledge of the signature structure of template invokables and the private AcceptsBlocks type. It also doesn't have an obvious analogue for templates defined in accompanying .hbs files rather than inline.

The proposal on the table in that RFC is to add a second type parameter to the base Component class that captures the blocks and corresponding parameter types a component intends to yield. This works well from a documentation perspective, but throws a wrench in the current inference-based approach to typing blocks that glint employs.

The two options are effectively explicit vs implicit typing for yields, and they can't easily coexist.

This Change

This PR embraces explicit typing for component yields. Aside from working better for documentation purposes, this has several other advantages:

  • It helps keep developer intent in line with reality, in the same way many teams choose to require function return type annotations even though the compiler can infer them.
  • It gives us the ability to plumb better information (like doc-comments and parameter names) through to block usage in templates, since we no longer need to do backflips to infer the blocks' return types.
  • It simplifies the 'flow' of types. The backing class and the template no longer impact one another's types; instead the backing class contains all relevant type information and that acts as a constraint on what's legal in the template.

The practical upshot is this: including a {{yield}} in a template is now a type error by default, and developers must explicitly provide the expected yields in order for the template to typecheck.

import Component, { hbs } from '@glimmerx/component';
import { AcceptsBlocks } from '@glint/template/-private';

export interface MyComponentArgs<T> {
  values: Array<T>;
}

export interface MyComponentYields<T> {
  default: [value: T, index: number];
  inverse: [];
}

export class MyComponent<T> extends Component<MyComponentArgs<T>, MyComponentYields<T>> {
  public static template = hbs`
    {{#each @values as |item index|}}
      {{yield item index}}
    {{else}}
      {{yield to="inverse"}}
    {{/each}}
  `;
}

Notes

Type Params Form Factor

What I've got here (and what the pre-RFC linked above suggests) has args and yields as two separate type parameters. They could also be combined into a single parameter without impacting anything fundamental about how this works (changes would only be necessary in the environment-specific typings, e.g. @glint/environment-glimmerx, nothing deeper).

As we look toward the future and additional constraints that it may be useful to capture, it may prove more ergonomic (and easier to expand down the line) to combine these things into a single signature type, along the lines of:

export interface MyComponentSignature<T> {
  element: HTMLUlistElement,
  args: { values: Array<T> },
  yields: {
    default: [value: T, index: number];
    inverse: [];
  }
}

export class class MyComponent<T> extends Component<MyComponentSignature<T>> {
  public static template = hbs`
    <ul ...attributes>
      {{#each @values as |value index|}}
        <li>{{! ... }}</li>
      {{/each}}
    </ul>
  `;
}

This would make it easier to mix-n-match what information is specified ("this component never yields, but splats its attrs on to an HTMLTableElement", or "this takes no args but always yields a string"), which otherwise gets cumbersome with an ever-growing list of positional type params.

Further Simplification

Making the signature for a component completely explicit and no longer dependent on the template and/or on inference has the potential to simplify a few other parts of glint in nice (though potentially breaking-change-inducing) ways.

This PR is just the minimal work needed to switch over the core mechanism for typing blocks, and those other simplifications will come as follow-on PRs, likely before I cut an 0.3.0 release.

@dfreeman dfreeman added the breaking A breaking change label Sep 6, 2020
@dfreeman dfreeman merged commit 74212b7 into master Sep 6, 2020
@dfreeman dfreeman deleted the blocks-rework branch September 6, 2020 17:30
@chriskrycho
Copy link
Member

I like this change a lot. I'm also sold on making components have a single descriptive and expandable type parameter. We should be able to codemod it easily enough.

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

Successfully merging this pull request may close these issues.

2 participants