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

Component signature #385

Merged
merged 7 commits into from
Mar 29, 2022
Merged

Component signature #385

merged 7 commits into from
Mar 29, 2022

Conversation

chriskrycho
Copy link
Contributor

Adds backwards-compatible support for component Signatures per Ember RFC #0748. 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.

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, but Glint users will benefit directly once Glint releases an update which can requires a version of @glimmer/component incorporating this change.

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.

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
Copy link
Contributor Author

Would love a review not only from core Glimmer folks but also from @dfreeman.

interface FullLongSig {
Args: {
Named: Args;
Positional: [];
Copy link
Member

Choose a reason for hiding this comment

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

😢

@chadhietala
Copy link
Member

LGTM

This provides the expected resolution within a component, but does not
introduce a breaking change by requiring all callers to pass a type
parameter.

Co-authored-by: James C. Davis <jamescdavis@gmail.com>
Copy link
Contributor

@dfreeman dfreeman left a comment

Choose a reason for hiding this comment

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

This looks great; thanks for getting it implemented @chriskrycho! Just a couple questions around type visibility that the answer to might be 'nope, this is exactly how we want it already'

test/types/component-test.ts Outdated Show resolved Hide resolved
packages/@glimmer/component/addon/-private/component.ts Outdated Show resolved Hide resolved
@chriskrycho
Copy link
Contributor Author

Note for myself: looks like once we land this I'll need to back-port it to the v1.x branch so we can cut a v1.1 release.

Co-authored-by: Dan Freeman <dfreeman@salsify.com>
@chriskrycho chriskrycho merged commit 2696e52 into master Mar 29, 2022
@chriskrycho chriskrycho deleted the component-signature branch March 29, 2022 14:40
chriskrycho added a commit that referenced this pull request Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants