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

Fix problem with transitive generics in Signatures #613

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gossi
Copy link
Collaborator

@gossi gossi commented Aug 17, 2023

Potential fix to #610

I was applying the fix as part of this PR locally on my PR tildeio/ember-element-helper#107 and see if in my type-test-component:

import Component from '@glimmer/component';
import type { ElementSignature, ElementFromTagName } from 'ember-element-helper';

interface ElementReceiverSignature<T extends string = 'article'> {
  Element: ElementFromTagName<T>;
  Args: {
    tag: ElementSignature<T>['Return'];
  };
  Blocks: {
    default: [];
  }
}

export default class ElementReceiver<T extends string> extends Component<ElementReceiverSignature<T>> {}
{{#let @tag as |Tag|}}
  {{!@glint-ignore}}
  <Tag id="content" ...attributes>{{yield}}</Tag>
{{/let}}

I was able to remove the line {{!@glint-ignore}} and if the type linting still passes, which is the case.

So, yes this is a potential fix, but I share the same concerns as @dfreeman this might come with side-effects.

So I'll make it a draft PR.

@chriskrycho
Copy link
Member

Thanks for opening this! The fact that CI is ✅ is a great initial sign – one of us will try to review in detail shortly.

@dfreeman
Copy link
Member

This should include, at a minimum, a new test case for the scenario this is intended to fix. Otherwise there's nothing to prevent us from flipping this back in the future and causing the same problem again

(I'd also recommend coming up with a more specific title for this PR, since that's what will go in the release notes—we clearly already "allow generics in signatures" in the general case, since many built-ins like let and each require that 😉)

Comment on lines +48 to +49
? NonNullable<Element> extends never
// ? Element extends null
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is a temporary revert. That is to write failing tests, which when switching back to the fix should be resolved.

Positional: []
}>();

expectTypeOf<ComponentSignatureArgs<ElementReceiverSignature<null>>>().toEqualTypeOf<{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, this line is the problematic part here, I'd say. For what I think needs to be tested, is how the components are invoked, rather than the static definition?

That is, in invocation, this should be null. But this is obviously a failure:

Type 'null' does not satisfy the constraint 'string'.

To my knowledge, to properly test this, is to provide broken types. How to do that?

@gossi
Copy link
Collaborator Author

gossi commented Sep 9, 2023

I added some tests. I copied over all the relevant types from (element) helper as well as a component receiving it. Feels a lot, but makes it accurate. If this can be less types, please let me know.

I also left comments where I got stuck. I think, I need to test broken/invalid types, in a environment with correct types?

@gossi gossi changed the title Allow generics in Signatures Fix problem with transitive generics in Signatures Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants