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

RFC: Glimmer component Signature type #748

Merged
merged 13 commits into from
Mar 25, 2022

Conversation

dfreeman
Copy link
Contributor

@dfreeman dfreeman commented May 13, 2021

Super-abridged summary: for TypeScript, this RFC proposes to change GlimmerComponent's Args type parameter to a Signature type that can capture richer information about how a component can be invoked.

Rendered text

@scottmessinger
Copy link
Contributor

I've been using Glint (and loving it) and didn't realize Element was where ....attributes goes. I'm +1 on a new name. Maybe AttributesElement or ElementReceivingAttributes or ElementReceivingSplattributes

@NullVoxPopuli
Copy link
Contributor

ElementReceivingAttributes or AttributesElement sounds good to me 💯

@wagenet
Copy link
Member

wagenet commented May 13, 2021

I think I prefer AttributesElement. It's clear enough (to me at least!) without feeling overly verbose.

@wagenet
Copy link
Member

wagenet commented May 13, 2021

I should also add that we've been using Glint for a few weeks and I love this approach. I'm fully on board.

@gossi gossi mentioned this pull request May 13, 2021
@jamescdavis
Copy link
Contributor

How about Splelement? 😆

@jamescdavis
Copy link
Contributor

jamescdavis commented May 14, 2021

But seriously, I do agree that Element isn't super clear and I like AttributesElement. I could also go for SplattributesElement to make it even clearer (but still not too verbose). Splattributes may be an informal name for attributes spread on an element, but it's well known and has even made it into an RFC: #435. I think people will know what AttributesElement means, but IMHO there's no doubt what a SplattributesElement is.

@lifeart
Copy link

lifeart commented May 14, 2021

AppliedAttributesElement | TransferredAttributesElement | HTMLAttributesTarget | HTMLAttributesNode | AttributesTarget

@chriskrycho
Copy link
Contributor

Obviously we can bikeshed all day on this one. 😂 The two things I'd note:

  • we should pick a name that is relatively short because TS (and JSDoc!) users will be typing it a lot
  • we should be careful not to paint ourselves into a corner with the name—I can imagine splitting apart modifiers and other attributes at some point (something I've been pondering a bit after a bunch of Octane adoption experience), and we'd want to make sure our names allowed for that kind of change without too much confusion

@chriskrycho
Copy link
Contributor

chriskrycho commented Jun 10, 2021

Some feedback from a discussion with Framework Core about this on 2021/05/28, with my own commentary added in:

  • There's a tricky bit of overlap between documenting the possible "target element(s)" for a given item and providing an accurate type for those. For example, in the case where attributes may or may not be applied (depending on the arguments passed or other state in the system) it might make sense for documentation purposes to specify Element: HTMLDivElement | null, to make clear to consumers what they can(not) rely on in terms of ...attributes placement. However, in that same scenario, passing any modifier will fail to type check, because the type of a modifier always expects the first argument to be Element (or a narrower type).

  • @chancancode was concerned about the fact that providing a type representing the union of potential targets for ...attributes would cause some modifier applications at the component invocation site not to type check. For example if the component specifies Element: HTMLMediaElement | HTMLDivElement, then application of a modifier which requires HTMLMediaElement will fail to type check.

    This is not only true, it's also expected and desirable: indeed, we might say it's the whole point! Appplying a modifier to a component is the same as passing a callback to a function (and that's exactly how tools like Glint represent things under the hood). Those have the same property: they will not type check if you pass a function which is not a strict super-type of the declared type of the callback accepted.

    Accordingly, I'm recommending we update the text of the RFC in the following ways:

    • Making the symmetry with callbacks explicit.
    • Showing how a user invoking a component can handle this, e.g. by writing a modifier which is applicable regardless (e.g. by accepting Element and narrowing).
    • Showing how a component author can handle it in the case where it can be statically known from the signature based on arguments (e.g. by providing a signature which is a union type and accepts the )
    • Noting that tooling (like Glint) can (and indeed Glint already does) provide the same kinds of escape hatches which TS itself does, via expect-error or ignore pragmas.
  • In addition to the expected discussion about the right name for BlockParams, there was a discussion of whether we should provide nicer ergonomics for the most common case of having a single default block yielded from the consumer. In that case, folks suggested something like this might be nicer:

    interface Signature {
    + BlockParams: [string, number]
    - BlockParams: {
    -   default: [string, number]
    - }
    }

    I think we could make this work with e.g. Glint in a fairly straightforward way, where the short form is equivalent to the longer form. This also dovetails nicely with the other point raised as part of the discussion: how we might support further extensions to this design to support additions to blocks in the future. This is not something this RFC needs to support, in my opinion, other than to note that the design is extensible. We can imagine that having exactly the same kind of behavior as the shorthand for the default block, that is: being able to accept just a list of block parameters or being able to expand it into something else. (I'm using Blocks here, since BlockParams would be too narrow a name.)

    // simple `default` block yielded
    interface Signature {
      Blocks: [string];
    }
    
    // multiple named blocks:
    interface Signature {
      Blocks: {
        default: [string];
        header: [];
        closeButton: [{ onClose: () => void }]
      }
    }
    
    // a hypothetical future expansion supporting passing attributes; note the
    // symmetry with Signature itself
    interface Signature {
      Blocks: {
        default: [string];
        header: {
          Element: HTMLHeadingElement;
          Yields: []
        };
        closeButton: [{ onClose: () => void }]
      }
    }
  • There was also interest in avoiding the use of the default name for the default block specificiation where possible. My own point of view here is that this doesn't make sense: default is public API and the signature of a component written in TS should use exactly the same semantics as we've defined in general. This RFC is not the place to hash out future direction for block names, and the design proposed here could readily be codemodded in the future if some other RFC deprecated default or otherwise changed the public API.

@chriskrycho
Copy link
Contributor

chriskrycho commented Jun 25, 2021

Discussion in the 2021-06-25 Framework Core Team meeting: folks are generally happy with where this is at. @chancancode noted that he doesn't care for the name BlockParams because it doesn't fit the way he wants to think about teaching what a component invocation is, and he and @pzuraq proposed that we use Blocks with a (at least for now) notional desugaring from this:

interface SignatureA {
  Blocks: [string];
}

interface SignatureB {
  Blocks: {
    header: [];
    body: [string];
  }
}

to this:

interface SignatureA {
  Blocks: {
    default: {
      Params: [string]
    }
  };
}

interface SignatureB {
  Blocks: {
    header: {
      Params: [];
    };
    body: {
      Params: [string];
    };
  }
}

This would then be "forward-looking" in that it supports future expansion of the idea of Blocks without change, and it would also map to the way Godfrey (and perhaps others!) think of the model here.

I'm leaving this comment as representative of that discussion, not representative of my own opinion here. I expect we'll probably have some further discussion on this thread on that point, though!

@chriskrycho
Copy link
Contributor

chriskrycho commented Jun 25, 2021

For my part, I disagree with this approach pretty strongly, for two reasons:

  1. I think that Blocks is basically unmotivated in terms of the actual behavior of the framework today.
  2. Using Blocks substantially increases the pedagogical burden for teaching people how to think about a component's signature.

I'll start with (2) and then circle back to (1). When reading Blocks: [string, number], it's much less unclear compared to either Yields or BlockParams what the types in the tuple represent. (That goes for the expanded form with named blocks as well as the default case.) Teaching how to use TS with Ember components therefore would require teaching the (notional) desugaring into the expanded format. Minimally, our coverage in Glint as well in official docs would need to explain that the tuple of types for Blocks actually represent the parameters yielded into each block—which in turn would naturally raise the question of why they aren't just named accordingly. But the only motivation we can supply there the hypothetical possibility of future expansion in that direction.

That leads me to (1): while I think it's good to design in a way that allows us to expand in the future, I do not believe we should design this to have a worse pedagogical experience now for something which may or may not ever happen. What's more, the use of BlockParams (or Yields) now allows us to move to Blocks in the future should it become desirable, and in fact to do so in exactly the same kind of "desugaring". In particular, we should consider that it's possible we never expand blocks, in which case we would be stuck with a notional desugaring in our pedagogy that doesn't pay for itself whatsoever—whereas the migration path if we do expand blocks is trivially codemoddable to any such future expansion.

This asymmetry of costs means that I think very strongly that we should not use Blocks here. In my view, at most we should note that BlockParams (or Yields, which I still prefer as a more accurate name for what is actually happening in a template!) could easily be migrated to Blocks iff future work requires it.

@lifeart
Copy link

lifeart commented Jun 26, 2021

There is a lot of confusion about it, especially, comparing with other frameworks and standards.

In templates we have "yield", but we have "has-block" helper,
in documentation it named "named block", but it may be "unnamed" (aka "default")

Syntaxis will be different for both.

In other frameworks, same behaviour has name "slot".

vue, angular, svelte

In web API, it's also "slot" - https://developer.mozilla.org/en-US/docs/Web/API/Element/slot

It's literally hell for any new developers.


Should we simplify overall API surface? (array of params, hashes and such, both on the same time used by 20% of developers, but brings strong overhead across ecosystem)

helper(unknown[], Record<string,unknown>) -> helper(...unknown[])
modifier(node, unknown[], Record<string,unknown>) -> modifier(node, ...unknown[])

Alternative version of naming (and default slot remains just 'slot')

interface SignatureA {
  Slot: [string, number]
}

interface SignatureB {
  Slots: {
    header: [ ],
    body: [ string ]
  }
}

interface SignatureC {
  Slot: [ number ],
  Slots: {
    header: [ ],
    body: [ string ]
  }
}

also, I like @chriskrycho reference about yields, and it may looks like:

interface SignatureA {
  yield: [string, number]
}

interface SignatureB {
  yields: {
    header: [ ],
    body: [ string ]
  }
}

interface SignatureC {
  yield: [ number ],
  yields: {
    header: [ ],
    body: [ string ]
  }
}

@chriskrycho
Copy link
Contributor

@lifeart yeah, we all spent some time thinking about the "slot" nomenclature a while back, and ultimately came down against because the behavior of slots in Vue, Svelte, and the web API is different enough from what named blocks do that it doesn't map in well. I believe there was some discussion of this on the original named blocks RFC as well. In any case, we should make the names used in the types match the names used by the framework, so the only real options are some combination of "blocks" and "yields" and maybe "params" with those. (If we renamed them at the framework level, we would of course also rename them in the types, but that's a different discussion for sure.)

@gossi
Copy link

gossi commented Jun 28, 2021

I think there are two perspectives to it:

  • As an author, I use the mechanism of yield to temporarily give control back
  • As a consumer, I use named(!) blocks to control the contents of my component

Params are a mechnanics to transport data from inside the component to the outside.

Also, my direction of reading is an author writes the type signature for the consumers, in the "language" of the consumers.

@dfreeman
Copy link
Contributor Author

dfreeman commented Jul 1, 2021

Thank you for writing up the Framework Core notes, @chriskrycho!

I do think it's worth noting that Blocks was in fact the name we used for this key in very early iterations of prototyping how component types might work for Glint. It's a tempting name, since it's short and already close-at-hand in conceptual space for anyone familiar with the general programming model in Glimmer templates.

In the course of working on Glint and on this RFC, I went hunting for prior art as input for both modeling and nomenclature. Among other languages, our blocks system perhaps most closely resembles Ruby's (albeit with some notable distinctions, such as components accepting multiple blocks and having no notion of a block return value). Accordingly, the Ruby typechecker Sorbet seemed a promising place to look, since they similarly needed to account for how to statically describe what kind of block might be passed to a method.

However, Sorbet (and Ruby itself) treats blocks and yields as essentially sugar for passing and invoking a callback, while Glimmer blocks occupy an entirely different namespace from component args and can't be invoked in any way other than with a {{yield}}.

This in itself may provide useful insight, though. Suppose we had a script-y analog to the API for invoking a component in a template, where args are passed via an object literal and a default "block" is a callback. The equivalent to calling this member of the signature Blocks, then, would be to call it Callback.

interface ScriptyComponentSignature {
  Args: object;
  Callback: unknown[]
}

class ScriptyComponent<T extends ScriptyComponentSignature> {
  constructor(
    private args: T['Args'],
    private callback: (...params: T['Callback']) => void
  ) {}
}

We can see a hint of where that name might be misaligned above, but it becomes much more explicit when we go to define a component:

interface EachComponentSignature<T> {
  Args: { items: Array<T> };
  Callback: [item: T; index: number]; // 🤔
}

class EachComponent<T> extends ScriptyComponent<EachComponentSignature<T>> {
  // ...
}

new EachComponent({ items: ['A', 'B', 'C'] }, (item, index) => {
  console.log(`#${index + 1}: ${item}`);
});

Writing a tuple type [item: T; index: number] and calling that type Callback feels quite odd to me. That isn't the type of a callback at all; it's the type of the parameters that the component provides to the callback in question.

Drawing this parallel was what led to us adopting the term Yields before the first public release of Glint, which might be equivalent to using something like Passes in our ScriptyComponentSignature above. While Yields avoids the directional conflation we had with Blocks, there's been a reasonable amount of pushback from the community on the term, based on lines of reasoning ranging from grammatical to pedagogical to (the totally fair) "that just doesn't fit my mental modal".

Similar to using a name like CallbackParams in our ScriptyComponentSignature above, the name BlockParams proposed in this RFC attempts to zoom in on a specific touchpoint between component authors and component consumers. It avoids making any reference to mechanical details on either side of that divide, while still aligning with terminology used by existing framework APIs like (has-block-params).

  1. I think that Blocks is basically unmotivated in terms of the actual behavior of the framework today.
  2. Using Blocks substantially increases the pedagogical burden for teaching people how to think about a component's signature.

I pretty strongly agree with both of @chriskrycho's points here. Future extensibility is absolutely an important aspect of API design, but an appropriate narrowly-scoped name still allows a straightforward migration path should we need it in the future without forcing a pedagogical model that requires explaining hypothetical future extensions to make sense today.

@dfreeman
Copy link
Contributor Author

dfreeman commented Jul 1, 2021

To make one thing clear that came up in the strike team call just now, the specific case that doesn't sit well with me for Blocks is when it combines with the default shorthand:

interface EachSignature<T> {
  // ...
  Blocks: [item: T; index: string];
}

interface SimpleWrappingSignature {
  // ...
  Blocks: [];
}

On its own I like the default shorthand a lot, as it directly mirrors the way templates themselves work—there's a special shorthand form for the very common use case of only working with a single default block, while a slightly more verbose form allows working with multiple named blocks.

In combination with the Blocks key, though, it leads to the situation @chriskrycho described, where you have mentally walk through multiple expansions to reach the intended meaning of the type. Blocks: [] in particular doesn't easily read to me as "accepts a single default block", which is what it would mean here.


### `Element`

The `Element` member of the signature declares what type of DOM element(s), if any, this component may be treated as encapsulating. That is, setting a non-`null` type for this member declares that this component may have HTML attributes applied to it, and that type reflects the type of DOM `Element` object any modifiers applied to the component will receive when they execute.
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the correct type for a custom element? The class of the custom element?

Copy link
Contributor

Choose a reason for hiding this comment

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

That’s correct. Note that the corresponding class is what’s used for regular HTML elements, too, so: HTMLAnchorElement, for an a tag, for example.

Comment on lines +31 to +34
2. How can I nest content within this component?
- Does it accept a default block? Named blocks?
- In what context will my block content appear in the component's output?
- For any blocks I write, what parameters does the component expose to them?
Copy link
Contributor

Choose a reason for hiding this comment

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

The current proposal does not seem to address all of these questions. The provided type information does not give a hint about the elements allowed within the block in all cases. E.g. if the component yields within a button element, a developer must only use phrasing content, which is not interactive.

In most cases this could be derived from the Element type. But only the component also applies splat attributes to that element. This may not be desirable in all cases.

Also there is the case, in which a parent element, limits the permitted content of a child element. This is the case for any element, which allows transparent content (e.g. <del> and <ins>). In that case permitted content can not be derived from that element alone.

Not sure if that must be addressed by this RFC though. Maybe it is enough to explicitly name this limitation in the RFC.

Copy link
Contributor

@chriskrycho chriskrycho Jan 30, 2022

Choose a reason for hiding this comment

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

The current proposal does not seem to address all of these questions.

It does; the additional very interesting point you raise about children is an additional point. While we could revise the RFC to make it explicit, the answer is implicit in the semantics of templates and unaffected by this proposal: they are simply functions of HTML. This proposal is not trying to specify a way to guarantee that no invalid HTML can be written. It is simply interested in extending the set of things which can be statically known about it for documentation and type checking purposes.

It might be possible to add in a further layer of analysis to accomplish that kind of analysis at a later point, but it is not a current goal for this doc or even for Glint. And it’s worth note that with this proposal and Glint, we are already doing more than any other JS framework. (The one ecosystem I think has taken a swing at the “allowed children” problem is PureScript, though I don’t know if their exploration in that space ended up broadly used even there. It is a very difficult problem because there is no inherent relationship between parent and child elements in HTML syntactically, only semantically.)

@chriskrycho
Copy link
Contributor

Update for folks watching this issue as eagerly as I am: we very briefly discussed this in the today’s Framework Core team meeting and are planning to have it resolved within the next few weeks (hopefully, though of course this being software there are no guarantees, by the end of the month!). I fully expect we’ll come out of that with a design we’re all satisfied with, and ready to move forward. I’ll update again as soon as we get there!

@simonihmig
Copy link
Contributor

simonihmig commented Mar 21, 2022

First, I love this! 😍

These interfaces are not importable, because importing them does not give any value to consumers: use of extends does not add any constraints (because of the optional-ity discussed below).

What I don't quite get is the reason for this...

When I author the signature of a component, I would like

  • to get autocompletion. E.g. I could type an uppercase B, and the IDE would expand this to Blocks. Hey, I'm old, I don't want to remember things! 😉
  • rule out as much as possible the likelihood for typos. Even if all the (root level) keys are optional, having the interface constrained would at least prevent me from adding wrong keys, e.g. accidentally writing Block.

Take the following (intentionally broken) example:

export interface NoticeSignature {
  Element: 'foo';
  Args: 'bar';
  /**
   * Any default block content will be displayed in the body of the notice.
   * This block receives a single `dismiss` parameter, which is an action
   * that can be used to dismiss the notice.
   */
  Block: {
    default: [dismiss: () => void];
  };
}

export default class Notice extends Component<NoticeSignature> {
  // ...
}

If you paste that into the TS playground linked in the RFC, TS wouldn't complain at all. For Args this is less an issue, as my wrong typing will trigger errors as soon as I actually use this.args, but would it be true for Block[s]?

Had I been able to to write this as export interface NoticeSignature extends GlimmerComponentSignature then I would immediately see all my silly errors.

We don't need to enforce users doing that, but why are we "actively" preventing it?

@dfreeman
Copy link
Contributor Author

dfreeman commented Mar 21, 2022

Thanks for the feedback, @simonihmig!

The problem with providing a base interface is that extend-ing one interface in another is different from implement-ing an interface in a class declaration. Though they seem very similar at first glance, the intuitions users might have from the latter don't actually apply to the former—your specific questions here are a great example of that in action! 🙂

to get autocompletion. E.g. I could type an uppercase B, and the IDE would expand this to Blocks.

TypeScript doesn't provide completions from a super interface, since the typical use case for one interface to extend another is to add new things rather than refine existing ones.

no completions for super fields in a child interface

having the interface constrained would at least prevent me from adding wrong keys, e.g. accidentally writing Block

For the same reason as outlined above, TypeScript has no issue with you adding extra keys that aren't part of the parent interface—after all, that's the point of extending an interface—so you won't get an error if you make a typo like Block instead of Blocks.

Ultimately, writing interface Mine extends Base { ... } is less like class Mine implements Base { ... } and much closer to writing type Mine = Base & { ... }. Because of that, exporting base interfaces doesn't actually let us provide much in the way of guardrails (and worse, gives people a false sense of security that we can!), so we've opted to keep them conceptual instead.

@simonihmig
Copy link
Contributor

The problem with providing a base interface is that extend-ing one interface in another is different from implement-ing an interface in a class declaration.

Oh geez, you are totally right! I was confusing that terribly somehow... 🙈

chriskrycho added a commit to ember-modifier/ember-modifier that referenced this pull request Mar 21, 2022
Details
-------

Following the design pioneered in emberjs/rfcs#748, introduce a new
`Signature` type and use it in defining the types for both function-
and class-based modifiers. This allows end users to write, for example:

    interface PlaySig {
      Args: {
        Named: {
          when: boolean;
        }
      }
      Element: HTMLMediaElement;
    }

    export default modifier<PlaySig>((el, _, { when: shouldPlay }) => {
      if (shouldPlay) {
        el.play();
      } else {
        el.pause();
      }
    });

This supports both `Named` and `Positional` args, and works equally
well with class-based modifiers. It also provides the `Element` type as
`Element` by default, and provides internal type utilities as well as
one external-facing type utility to make writing the type of `args`
when passed to a modifier `constructor` easy to do in terms of the
`Signature` which represents the modifier.

Users do not *need* to type this form: the form using basic inference
continues to work as well:

    export default modifier((
      el: HTMLMediaElement,
      _: [],
      { when: shouldPlay }: { when: boolean }
    ) => {
      if (shouldPlay) {
        el.play();
      } else {
        el.pause();
      }
    }));

That is: the support for `Signatures` is strictly *new* capability
which was not present before.

The same kind of signature works with class-based modifiers as well,
while retaining backward compatibility with the `Args`-based signature
and declaration on the subclass `element` field directly. To support
that in a way that does not require users to name their args over and
over again, introduce an `ArgsFor` type utility which translates from the signature type to the runtime form:

    import Modifier, { ArgsFor } from 'ember-modifier';

    interface PlaySig {
      Args: {
        Named: {
          when: boolean;
        };
      };
      Element: HTMLMediaElement;
    }

    class MyModifier extends Modifier<PlaySig> {
      constructor(owner: unknown, args: ArgsFor<PlaySig>) {
        super(owner, args);
        // ...
      }

      didReceiveArguments() {
        const shouldPlay = this.args.named.when;
        if (shouldPlay) {
          this.element.play();
        } else {
          this.element.pause();
        }
      }
    }

Additionally, the `modifier` function now returns an opaque
`FunctionBasedModifier` type. This mostly exists to provide nice hooks
for tooling to hook onto, but it has the added benefit of showing
something besides `unknown` in an editor, and that "something" shows
the arguments and element specified for the modifier.

Supporting changes
------------------

-   Update the README to match, and fix a *lot* of long-since outdated
    documentation along the way.

-   Loosen type test constraint to support older TS versions

    The new type signature *is* compatible (as the tests on TS 4.5+
    show), but on older versions of TS, it does not check under a type
    equality check, only under a type matching check. For the purposes
    of the specific test in question, that's perfectly fine, because
    the *other* type tests cover inference and resolution correctly.
chriskrycho added a commit to ember-modifier/ember-modifier that referenced this pull request Mar 21, 2022
Details
-------

Following the design pioneered in emberjs/rfcs#748, introduce a new
`Signature` type and use it in defining the types for both function-
and class-based modifiers. This allows end users to write, for example:

    interface PlaySig {
      Args: {
        Named: {
          when: boolean;
        }
      }
      Element: HTMLMediaElement;
    }

    export default modifier<PlaySig>((el, _, { when: shouldPlay }) => {
      if (shouldPlay) {
        el.play();
      } else {
        el.pause();
      }
    });

This supports both `Named` and `Positional` args, and works equally
well with class-based modifiers. It also provides the `Element` type as
`Element` by default, and provides internal type utilities as well as
one external-facing type utility to make writing the type of `args`
when passed to a modifier `constructor` easy to do in terms of the
`Signature` which represents the modifier.

Users do not *need* to type this form: the form using basic inference
continues to work as well:

    export default modifier((
      el: HTMLMediaElement,
      _: [],
      { when: shouldPlay }: { when: boolean }
    ) => {
      if (shouldPlay) {
        el.play();
      } else {
        el.pause();
      }
    }));

That is: the support for `Signatures` is strictly *new* capability
which was not present before.

The same kind of signature works with class-based modifiers as well,
while retaining backward compatibility with the `Args`-based signature
and declaration on the subclass `element` field directly. To support
that in a way that does not require users to name their args over and
over again, introduce an `ArgsFor` type utility which translates from the signature type to the runtime form:

    import Modifier, { ArgsFor } from 'ember-modifier';

    interface PlaySig {
      Args: {
        Named: {
          when: boolean;
        };
      };
      Element: HTMLMediaElement;
    }

    class MyModifier extends Modifier<PlaySig> {
      constructor(owner: unknown, args: ArgsFor<PlaySig>) {
        super(owner, args);
        // ...
      }

      didReceiveArguments() {
        const shouldPlay = this.args.named.when;
        if (shouldPlay) {
          this.element.play();
        } else {
          this.element.pause();
        }
      }
    }

Additionally, the `modifier` function now returns an opaque
`FunctionBasedModifier` type. This mostly exists to provide nice hooks
for tooling to hook onto, but it has the added benefit of showing
something besides `unknown` in an editor, and that "something" shows
the arguments and element specified for the modifier.

Supporting changes
------------------

-   Update the README to match, and fix a *lot* of long-since outdated
    documentation along the way.

-   Loosen type test constraint to support older TS versions

    The new type signature *is* compatible (as the tests on TS 4.5+
    show), but on older versions of TS, it does not check under a type
    equality check, only under a type matching check. For the purposes
    of the specific test in question, that's perfectly fine, because
    the *other* type tests cover inference and resolution correctly.
chriskrycho added a commit to ember-modifier/ember-modifier that referenced this pull request Mar 21, 2022
Details
-------

Following the design pioneered in emberjs/rfcs#748, introduce a new
`Signature` type and use it in defining the types for both function-
and class-based modifiers. This allows end users to write, for example:

    interface PlaySig {
      Args: {
        Named: {
          when: boolean;
        }
      }
      Element: HTMLMediaElement;
    }

    export default modifier<PlaySig>((el, _, { when: shouldPlay }) => {
      if (shouldPlay) {
        el.play();
      } else {
        el.pause();
      }
    });

This supports both `Named` and `Positional` args, and works equally
well with class-based modifiers. It also provides the `Element` type as
`Element` by default, and provides internal type utilities as well as
one external-facing type utility to make writing the type of `args`
when passed to a modifier `constructor` easy to do in terms of the
`Signature` which represents the modifier.

Users do not *need* to type this form: the form using basic inference
continues to work as well:

    export default modifier((
      el: HTMLMediaElement,
      _: [],
      { when: shouldPlay }: { when: boolean }
    ) => {
      if (shouldPlay) {
        el.play();
      } else {
        el.pause();
      }
    }));

That is: the support for `Signatures` is strictly *new* capability
which was not present before.

The same kind of signature works with class-based modifiers as well,
while retaining backward compatibility with the `Args`-based signature
and declaration on the subclass `element` field directly. To support
that in a way that does not require users to name their args over and
over again, introduce an `ArgsFor` type utility which translates from the signature type to the runtime form:

    import Modifier, { ArgsFor } from 'ember-modifier';

    interface PlaySig {
      Args: {
        Named: {
          when: boolean;
        };
      };
      Element: HTMLMediaElement;
    }

    class MyModifier extends Modifier<PlaySig> {
      constructor(owner: unknown, args: ArgsFor<PlaySig>) {
        super(owner, args);
        // ...
      }

      didReceiveArguments() {
        const shouldPlay = this.args.named.when;
        if (shouldPlay) {
          this.element.play();
        } else {
          this.element.pause();
        }
      }
    }

Additionally, the `modifier` function now returns an opaque
`FunctionBasedModifier` type. This mostly exists to provide nice hooks
for tooling to hook onto, but it has the added benefit of showing
something besides `unknown` in an editor, and that "something" shows
the arguments and element specified for the modifier.

Supporting changes
------------------

-   Update the README to match, and fix a *lot* of long-since outdated
    documentation along the way.

-   Loosen type test constraint to support older TS versions

    The new type signature *is* compatible (as the tests on TS 4.5+
    show), but on older versions of TS, it does not check under a type
    equality check, only under a type matching check. For the purposes
    of the specific test in question, that's perfectly fine, because
    the *other* type tests cover inference and resolution correctly.
chriskrycho added a commit to ember-modifier/ember-modifier that referenced this pull request Mar 21, 2022
Details
-------

Following the design pioneered in emberjs/rfcs#748, introduce a new
`Signature` type and use it in defining the types for both function-
and class-based modifiers. This allows end users to write, for example:

    interface PlaySig {
      Args: {
        Named: {
          when: boolean;
        }
      }
      Element: HTMLMediaElement;
    }

    export default modifier<PlaySig>((el, _, { when: shouldPlay }) => {
      if (shouldPlay) {
        el.play();
      } else {
        el.pause();
      }
    });

This supports both `Named` and `Positional` args, and works equally
well with class-based modifiers. It also provides the `Element` type as
`Element` by default, and provides internal type utilities as well as
one external-facing type utility to make writing the type of `args`
when passed to a modifier `constructor` easy to do in terms of the
`Signature` which represents the modifier.

Users do not *need* to type this form: the form using basic inference
continues to work as well:

    export default modifier((
      el: HTMLMediaElement,
      _: [],
      { when: shouldPlay }: { when: boolean }
    ) => {
      if (shouldPlay) {
        el.play();
      } else {
        el.pause();
      }
    }));

That is: the support for `Signatures` is strictly *new* capability
which was not present before.

The same kind of signature works with class-based modifiers as well,
while retaining backward compatibility with the `Args`-based signature
and declaration on the subclass `element` field directly. To support
that in a way that does not require users to name their args over and
over again, introduce an `ArgsFor` type utility which translates from the signature type to the runtime form:

    import Modifier, { ArgsFor } from 'ember-modifier';

    interface PlaySig {
      Args: {
        Named: {
          when: boolean;
        };
      };
      Element: HTMLMediaElement;
    }

    class MyModifier extends Modifier<PlaySig> {
      constructor(owner: unknown, args: ArgsFor<PlaySig>) {
        super(owner, args);
        // ...
      }

      didReceiveArguments() {
        const shouldPlay = this.args.named.when;
        if (shouldPlay) {
          this.element.play();
        } else {
          this.element.pause();
        }
      }
    }

Additionally, the `modifier` function now returns an opaque
`FunctionBasedModifier` type. This mostly exists to provide nice hooks
for tooling to hook onto, but it has the added benefit of showing
something besides `unknown` in an editor, and that "something" shows
the arguments and element specified for the modifier.

Supporting changes
------------------

-   Update the README to match, and fix a *lot* of long-since outdated
    documentation along the way.

-   Loosen type test constraint to support older TS versions

    The new type signature *is* compatible (as the tests on TS 4.5+
    show), but on older versions of TS, it does not check under a type
    equality check, only under a type matching check. For the purposes
    of the specific test in question, that's perfectly fine, because
    the *other* type tests cover inference and resolution correctly.
chriskrycho added a commit to ember-modifier/ember-modifier that referenced this pull request Mar 21, 2022
Details
-------

Following the design pioneered in emberjs/rfcs#748, introduce a new
`Signature` type and use it in defining the types for both function-
and class-based modifiers. This allows end users to write, for example:

    interface PlaySig {
      Args: {
        Named: {
          when: boolean;
        }
      }
      Element: HTMLMediaElement;
    }

    export default modifier<PlaySig>((el, _, { when: shouldPlay }) => {
      if (shouldPlay) {
        el.play();
      } else {
        el.pause();
      }
    });

This supports both `Named` and `Positional` args, and works equally
well with class-based modifiers. It also provides the `Element` type as
`Element` by default, and provides internal type utilities as well as
one external-facing type utility to make writing the type of `args`
when passed to a modifier `constructor` easy to do in terms of the
`Signature` which represents the modifier.

Users do not *need* to type this form: the form using basic inference
continues to work as well:

    export default modifier((
      el: HTMLMediaElement,
      _: [],
      { when: shouldPlay }: { when: boolean }
    ) => {
      if (shouldPlay) {
        el.play();
      } else {
        el.pause();
      }
    }));

That is: the support for `Signatures` is strictly *new* capability
which was not present before.

The same kind of signature works with class-based modifiers as well,
while retaining backward compatibility with the `Args`-based signature
and declaration on the subclass `element` field directly. To support
that in a way that does not require users to name their args over and
over again, introduce an `ArgsFor` type utility which translates from the signature type to the runtime form:

    import Modifier, { ArgsFor } from 'ember-modifier';

    interface PlaySig {
      Args: {
        Named: {
          when: boolean;
        };
      };
      Element: HTMLMediaElement;
    }

    class MyModifier extends Modifier<PlaySig> {
      constructor(owner: unknown, args: ArgsFor<PlaySig>) {
        super(owner, args);
        // ...
      }

      didReceiveArguments() {
        const shouldPlay = this.args.named.when;
        if (shouldPlay) {
          this.element.play();
        } else {
          this.element.pause();
        }
      }
    }

Additionally, the `modifier` function now returns an opaque
`FunctionBasedModifier` type. This mostly exists to provide nice hooks
for tooling to hook onto, but it has the added benefit of showing
something besides `unknown` in an editor, and that "something" shows
the arguments and element specified for the modifier.

Supporting changes
------------------

-   Update the README to match, and fix a *lot* of long-since outdated
    documentation along the way.

-   Loosen type test constraint to support older TS versions

    The new type signature *is* compatible (as the tests on TS 4.5+
    show), but on older versions of TS, it does not check under a type
    equality check, only under a type matching check. For the purposes
    of the specific test in question, that's perfectly fine, because
    the *other* type tests cover inference and resolution correctly.
chriskrycho added a commit to glimmerjs/glimmer.js that referenced this pull request Mar 23, 2022
Adds backwards-compatible support for component `Signature`s per [Ember
RFC #0748][rfc]. The public API of `Component` is now *more* permissive
than it was previously, because it changes the type parameter to be an
unconstrained generic `<S>` (for "signature") which can accept *either*
the existing `Args` types *or* a new `Signature` which includes `Args`
and adds `Blocks` and `Element`.

[rfc]: emberjs/rfcs#748

The `Args` part of the new signature work exactly like the old
args-only type did. The `Blocks` and `Element` parts of a signature are
inert from the perspective of TypeScript users who are not yet using
[Glint][glint], but Glint users will benefit directly once Glint
releases an update which can requires a version of `@glimmer/component`
incorporating this change.

[glint]: https://github.com/typed-ember/glint

Since this change is backwards compatible, we can deprecate the
non-`Signature` form at a later time when we are ready for a 2.0
release.

To validate these changes, with the relatively complicated type
machinery they require under the hood, this also introduces the
`expect-type` type testing library, rewrites the existing type tests
using it, and introduces new type tests for all supported forms of the
`Signature`.
chriskrycho added a commit that referenced this pull request Mar 24, 2022
RFC #724 originally intentionally excluded template type checking via
Glint from its designation of "official support" for Ember. However, in
the time since the RFC was authored, there have been two significant changes:

1.  Glint itself has matured significantly, with no known major issues
    at this time (though plenty of polish still to be done).

2.  The *major* design issues in Ember needed to unblock Glint have
    been resolved:

    - resolution of template identifiers (components, helpers, and
      modifiers): RFC #779
    - a Glimmer Component type signature which exposes information
      about the blocks and the target for `...attributes`: RFC #748

Although there remain a number of smaller design questions to resolve,
this is sufficient for us to treat type-checked templates as a viable
part of our story, and given *viability*, we think this is *necessary*.
@rwjblue
Copy link
Member

rwjblue commented Mar 25, 2022

Great work getting this over the line, thank you for the awesome discussion here folks!

Let's do this!!!! 💪 🎉 💯

@rwjblue rwjblue merged commit 6acdda7 into emberjs:master Mar 25, 2022
chriskrycho added a commit to glimmerjs/glimmer.js that referenced this pull request Mar 29, 2022
Adds backwards-compatible support for component `Signature`s per [Ember
RFC #0748][rfc]. The public API of `Component` is now *more* permissive
than it was previously, because it changes the type parameter to be an
unconstrained generic `<S>` (for "signature") which can accept *either*
the existing `Args` types *or* a new `Signature` which includes `Args`
and adds `Blocks` and `Element`.

[rfc]: emberjs/rfcs#748

The `Args` part of the new signature work exactly like the old
args-only type did. The `Blocks` and `Element` parts of a signature are
inert from the perspective of TypeScript users who are not yet using
[Glint][glint], but Glint users will benefit directly once Glint
releases an update which can requires a version of `@glimmer/component`
incorporating this change.

[glint]: https://github.com/typed-ember/glint

Since this change is backwards compatible, we can deprecate the
non-`Signature` form at a later time when we are ready for a 2.0
release.

To validate these changes, with the relatively complicated type
machinery they require under the hood, this also introduces the
`expect-type` type testing library, rewrites the existing type tests
using it, and introduces new type tests for all supported forms of the
`Signature`.

(cherry picked from commit 967d028)
chriskrycho added a commit to glimmerjs/glimmer.js that referenced this pull request Mar 29, 2022
Adds backwards-compatible support for component `Signature`s per [Ember
RFC #0748][rfc]. The public API of `Component` is now *more* permissive
than it was previously, because it changes the type parameter to be an
unconstrained generic `<S>` (for "signature") which can accept *either*
the existing `Args` types *or* a new `Signature` which includes `Args`
and adds `Blocks` and `Element`.

[rfc]: emberjs/rfcs#748

The `Args` part of the new signature work exactly like the old
args-only type did. The `Blocks` and `Element` parts of a signature are
inert from the perspective of TypeScript users who are not yet using
[Glint][glint], but Glint users will benefit directly once Glint
releases an update which can requires a version of `@glimmer/component`
incorporating this change.

[glint]: https://github.com/typed-ember/glint

Since this change is backwards compatible, we can deprecate the
non-`Signature` form at a later time when we are ready for a 2.0
release.

To validate these changes, with the relatively complicated type
machinery they require under the hood, this also introduces the
`expect-type` type testing library, rewrites the existing type tests
using it, and introduces new type tests for all supported forms of the
`Signature`.

(cherry picked from commit 967d028)
@dfreeman dfreeman deleted the glimmer-component-signature branch April 7, 2022 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period T-framework RFCs that impact the ember.js library
Projects
None yet
Development

Successfully merging this pull request may close these issues.