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

ensure-safe-component breaks types #518

Closed
simonihmig opened this issue Jan 16, 2023 · 4 comments · Fixed by embroider-build/embroider#1367
Closed

ensure-safe-component breaks types #518

simonihmig opened this issue Jan 16, 2023 · 4 comments · Fixed by embroider-build/embroider#1367
Labels
question Further information is requested

Comments

@simonihmig
Copy link
Contributor

In a project I have Glint types working, where I pass a component constructor to {{component}}, which matches the expected WithBoundArgs defined for the block params, like this:

  {{yield
    (hash
      field=(component this.FieldComponent data=this.internalData
    )
  }}

Again, working fine! (Reproducible state in this commit)
Then, for Embroider compatibility, I am wrapping the component constructor with (ensure-safe-component):

  {{yield
    (hash
      field=(component
        (ensure-safe-component this.FieldComponent) data=this.internalData
      )
    )
  }}

I copied its type definition from its own source (which @chriskrycho added, but which are not released yet unfortunately). Its type definition looks good AFAICT, trying to preserve the signature of a passed component constructor.

However starting from here (in this commit, I am getting weird type errors, like Argument of type 'typeof HeadlessFormFieldComponent' is not assignable to parameter of type 'string | ComponentLike<unknown>'. (unknown!?) and Type 'void | DebuggerKeyword | HasBlockKeyword | HasBlockParamsKeyword | InElementKeyword | LetKeyword | ... 20 more ... | typeof HeadlessFormComponent' is not assignable to type 'InvokableConstructor<...> (which probably is a transitive error from the first?)

Full details
src/components/headless-form.hbs:4:7 - error TS2322: Type 'void | DebuggerKeyword | HasBlockKeyword | HasBlockParamsKeyword | InElementKeyword | LetKeyword | ... 20 more ... | typeof HeadlessFormComponent' is not assignable to type 'InvokableConstructor<(named: Omit<{ data: Partial<DATA>; name: keyof DATA; }, "data"> & Partial<Pick<{ data: Partial<DATA>; name: keyof DATA; }, "data">>) => AcceptsBlocks<...>>'.
  Type 'void' is not assignable to type 'InvokableConstructor<(named: Omit<{ data: Partial<DATA>; name: keyof DATA; }, "data"> & Partial<Pick<{ data: Partial<DATA>; name: keyof DATA; }, "data">>) => AcceptsBlocks<...>>'.

4       field=(component
        ~~~~~

  src/components/headless-form.ts:17:9
    17         field: WithBoundArgs<typeof FieldComponent<DATA>, 'data'>;
               ~~~~~
    The expected type comes from property 'field' which is declared here on type '{ field: InvokableConstructor<(named: Omit<{ data: Partial<DATA>; name: keyof DATA; }, "data"> & Partial<Pick<{ data: Partial<DATA>; name: keyof DATA; }, "data">>) => AcceptsBlocks<...>>; }'

src/components/headless-form.hbs:4:13 - error TS2769: No overload matches this call.
  The last overload gave the following error.
    Argument of type '{ data: Partial<DATA>; }' is not assignable to parameter of type 'Partial<EmptyObject>'.
      Object literal may only specify known properties, and 'data' does not exist in type 'Partial<EmptyObject>'.

4       field=(component
              ~~~~~~~~~~
5         (ensure-safe-component this.FieldComponent) data=this.internalData
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
6       )
  ~~~~~~~

  ../node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@glint/environment-ember-loose/-private/intrinsics/component.d.ts:64:3
     64   <
          ~
     65     Args,
        ~~~~~~~~~
    ... 
     74       | undefined
        ~~~~~~~~~~~~~~~~~
     75   ): null | (abstract new () => PartiallyAppliedComponent<Args, GivenArgs, Return>);
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    The last overload is declared here.

src/components/headless-form.hbs:5:32 - error TS2345: Argument of type 'typeof HeadlessFormFieldComponent' is not assignable to parameter of type 'string | ComponentLike<unknown>'.
  Types of construct signatures are incompatible.
    Type 'new <DATA extends object>(owner: unknown, args: { data: Partial<DATA>; name: keyof DATA; }) => HeadlessFormFieldComponent<DATA>' is not assignable to type 'abstract new (...args: any) => Invokable<(named: EmptyObject) => AcceptsBlocks<FlattenBlockParams<EmptyObject>, null>>'.
      Construct signature return types 'HeadlessFormFieldComponent<object>' and 'Invokable<(named: EmptyObject) => AcceptsBlocks<FlattenBlockParams<EmptyObject>, null>>' are incompatible.
        The types of '[Invoke]' are incompatible between these types.
          Type '(named: { data: object; name: never; }) => AcceptsBlocks<FlattenBlockParams<{ default: { Params: { Positional: [{ label: InvokableConstructor<(named: Omit<{ fieldId: string; }, "fieldId"> & Partial<Pick<{ fieldId: string; }, "fieldId">>) => AcceptsBlocks<...>>; input: InvokableConstructor<...>; }]; }; }; }>, null>' is not assignable to type '(named: EmptyObject) => AcceptsBlocks<FlattenBlockParams<EmptyObject>, null>'.
            Types of parameters 'named' and 'named' are incompatible.
              Type 'EmptyObject' is missing the following properties from type '{ data: object; name: never; }': data, name

5         (ensure-safe-component this.FieldComponent) data=this.internalData

Full CI log here

Interestingly I was able to fix this by casting the assignment of the component constructor (in the backing class) to an explicit ComponentLike<...> (in this commit).

Seems there are nitty gritty differences between ComponentLike<MyComponentSignature> and typeof MyComponent, that come into play here, right? Is that expected?

@dfreeman
Copy link
Member

I think there are a few things going on here, all of which are interplaying to lead to this error:

  • Glint 0.9 has some unintuitive behaviors around what is or isn't assignable to ComponentLike<T> — this was cleaned up in Validate assignability rules for ComponentLike and friends #470 as part of the preparation for 1.0
  • I think the ensureSafeComponent function you pulled from Embroider is meant to be called from JS/TS code (note how it takes a second argument to pull an Owner from); for a template you'd want the EnsureSafeComponentHelper class declared a couple lines further down
  • That said, I think the S parameter in that EnsureSafeComponentHelper signature is causing problems, because it gets generalized to unknown, and ComponentLike<unknown> is not "a ComponentLike you can use however", but rather the same as bare ComponentLike or ComponentLike<{}>: a component with no args, blocks, or element. Eliminating S and changing to something like EnsureSafeComponentHelper<C extends string | ComponentLike<any>> may work better?

Finally, there's something subtle (and, I think, not directly Glint-related) going on with your FieldComponent type/property. As you noted, you needed to annotate that field as ComponentLike<...> for it to pass typechecking.

The reason for that is that you have two unique DATA type parameters (one in HeadlessFormComponent and one in HeadlessFormFieldComponent), and the way you have your defaulting for the key type set up, you're going to hit a type error unless you somehow tell the type system that DATA in the parent and child actually refer to the same thing. Adding the ComponentLike annotation as you've done is one option for that, but you could also more simply do FieldComponent = FieldComponent<DATA>;.

@dfreeman dfreeman added the question Further information is requested label Jan 23, 2023
@simonihmig
Copy link
Contributor Author

simonihmig commented Feb 24, 2023

Hi Dan, sorry for taking so long to get back to this, finally I have incorporated your feedback into CrowdStrike/ember-headless-form#57. So thanks for that, I think you pretty much nailed everything!

In order of your points:

  1. Updated to Glint 1.0.0-beta.3
  2. Correct, that was my fault.
  3. Indeed it (S = unknown) does cause problems. With the above two points fixed, I am seeing exactly that: the bare ComponentLike is not assignable to the Component that the assignment expects (args, blocks). I have redefined the EnsureSafeComponentHelper type here as suggested, and with that it works.
  4. you are right about my use of DATA. I believe I have tried what you suggested (FieldComponent = FieldComponent<DATA>) before, but not entirely sure anymore. But anyway, with the above points fixed, I tried again, and it works now also! 🎉

One note though about 3.:

I think due to ComponentLike<any> we are now loosing type safety when wrapping things in ensureSafeComponent, right? Before (the case of ComponentLike<unknown>) it would be like the most minimal component, that allows nothing (no args, no blocks). Now with ComponentLike<any> we get a component back that allows anything. But ideally the wrapped component (if the arg was not a string) would have the exact same shape as the original. And it seems to me this was the intention of the S generic type here, right?

With generic functions or classes, TS would be able to infer what S refers to (and not necessarily narrow it down to unknown). Is there no way for TS to do that also here, like inferring from Args.Positional[0] that the C we passed to it is not ComponentLike<unknown>, but the actual thing we passed to it, so what we return here is the same? /cc @chriskrycho

@dfreeman
Copy link
Member

I think due to ComponentLike<any> we are now loosing type safety when wrapping things in ensureSafeComponent, right? Before (the case of ComponentLike<unknown>) it would be like the most minimal component, that allows nothing (no args, no blocks).

T extends any is not the same as any — TS will happily infer a narrower T than any if it can, as you can see in this playground.

Now with ComponentLike<any> we get a component back that allows anything.

Are you sure? Did you check? 🙂
Per my point above, that's not the behavior I would expect, nor is it what I see in practice:

image

@simonihmig
Copy link
Contributor Author

Are you sure? Did you check? 🙂

I did not. 🙈
And you are right, again. Sorry for the confusion!

I will file a PR with the changes to @embroider/util then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants