typed-ember / glint

TypeScript powered tooling for Glimmer templates
https://typed-ember.gitbook.io/glint
MIT License
109 stars 51 forks source link

Using `fn` & `mut` inside conditional #569

Open charlesfries opened 1 year ago

charlesfries commented 1 year ago

I am seeing the following error while trying to use fn and mut to set a property inside of a conditional statement:

No overload matches this call.
  Overload 1 of 9, '(update: Mut<true>, value: true): () => void', gave the following error.
    Argument of type 'false' is not assignable to parameter of type 'true'.
  Overload 2 of 9, '(f: (a: boolean) => void, a: boolean): () => void', gave the following error.
    Argument of type 'Mut<true>' is not assignable to parameter of type '(a: boolean) => void'.glint(2769)

In the screenshot, the first example outside of the conditonal works fine, while the second inside the if yields the error.

Glint

It seems that Glint narrows the boolean type of showModal inside of the conditional to true, making false understandably unassignable to true. Am I using these helpers incorrectly here, or is this a valid issue?

"@glint/core": "^1.0.1",
"@glint/environment-ember-loose": "^1.0.1",
"@glint/template": "^1.0.1",
"ember-cli": "~4.10.0",
"ember-source": "~4.11.0",
node v16.15.0
jamescdavis commented 1 year ago

You are using the helpers correctly and this is a valid issue.

Glint is actually doing the "right thing" here types-wise, but it's inconvenient. Indeed, the if is narrowing this.showModal to true. The problem is that, because of the way mut works, it cannot be widened back out to boolean and allow false to be bound as the argument to the function returned by mut.

The mut helper is essentially typed as:

declare function mut<T>(target: T): (value: T) => void;

(not exactly, since it's a helper, but we can think of it simplified as this for illustrative purposes)

The type of the target becomes the type of the value parameter in the returned function. In your case, T is inferred to be true and thus the returned function expects value to be true.

Playground example: https://www.typescriptlang.org/play?#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXwFtkMAeAFQD4AKDWAcxAwC54yBKFqgNygmRBbt4AXgrwuOLMADcAKFkRGSHDgCELAEYrFUVHNlEMVRCrbHeAZxBt9WRPGOn4Ab1nxCxRzjOJL1uQC+QA

One way we could consider fixing this would be to use an overload to special-case types that extend boolean such that the type is always widened to include boolean: https://www.typescriptlang.org/play?#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXwFtkMAeAFXhAA8MRVgBneAIxxwhClQD4AKDWAHMQGAFzwyASnG8AblAjIQ4igB8WbDl0nwAvN3iycWYAG4AUKEiwEKdNjyFi5PgJjCxE6fDkKlKnX1DYzNzcw4MJDYAQnFWdk58dTRQRCxUEFDzIgxeRDZJPIUGEEkLcyxEH3ycHQBvc3gnXJrCxGLSiwBfIA

This gives up a tiny bit of type safety, but given that if you are binding a boolean to mut, you almost certainly want to be able to pass false even when narrowed to true and vice versa. IMHO we should only do this for booleans because we don't want to allow something like: https://www.typescriptlang.org/play?#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXwFtkMAeAFXhAA8MRVgBneBjGLVAcwD4AKDWDiAwAueGQCUongDcoEZCFEUAPs1bsO4+AF4u8aTizAA3AChQkWAhTpseQsXK9+MQSLGT4MuQqVbd+oYmpqYQQkg4OACEogDkiJGx8KqxAEawsWamRBg8CTjiPGlQAF6x4sZAA

@dfreeman, @chriskrycho, thoughts? Can you think of a better solution?

dfreeman commented 1 year ago

My inclination is to say we should leave this as-is. Widening specifically for booleans is clever, though it still puts you in situations where you may be breaking your intended invariants: https://www.typescriptlang.org/play?#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXwFtkMAeAFXhAA8MRVgBneAIxxwhClQD4AKDWAHMQGAFzwyASnG8AblAjIQ4igB8WbDl0nwAvN3iycWYAG4AUKEiwEKdNjyFi5PgJjCxE6fDkKlKnX1DYzNzS3BoOHgODHgcZgArcQBvShgYHBhxRAUGEAAaeDgGZAhPVGQCZhAYeABfeHVUmoys+AwYJUK4KAY8cQYOrFRBeotzLEQfeISAOhbMnWTzeCcMXhn59MXeHIg8yQs6oA

More than that, though, I worry that the inconsistency between booleans and other literal types will cause further confusion. If I have a property modalState: 'open' | 'closed', then this same problem rears its head again:

{{#if (eq this.modalState 'open')}}
  <Modal @onClose={{fn (mut this.modalState) 'closed'}} />
{{/if}}

As a user, if the boolean version worked, I'd be surprised to find that the string version didn't, but as you pointed out, the danger of widening string subtypes is a lot higher.

@jamescdavis if you and/or @chriskrycho feel strongly otherwise I'm happy to defer, but given the above I'd lean toward documenting this as a known rough edge with a workaround of defining a bespoke action instead of using mut.

jamescdavis commented 1 year ago

Yeah, I don't love the inconsistency that would be introduced by special-casing of booleans (even though I proposed it 😆). And my first instinct was also, "don't use mut" 🙂.

jamescdavis commented 1 year ago

But also, I'm having trouble reproing this. In a test app, I set all my versions the same as listed here and Glint is not complaining about {{fn (mut this.foo) false}} when this.foo is narrowed to true:

Screen Shot 2023-05-02 at 12 06 35 PM

but certainly does when you do something nonsensical:

Screen Shot 2023-05-02 at 12 07 52 PM Screen Shot 2023-05-02 at 12 10 34 PM

I'm not sure what I'm doing differently. ¯\_(ツ)_/¯

charlesfries commented 1 year ago

Here is a repro: https://github.com/charlesfries/glint-test I observe the error on line 2 of app/components/foo.hbs. Using Glint VSCode extension v1.0.3

https://github.com/charlesfries/glint-test/actions/runs/4863837378/jobs/8672084205

image

jamescdavis commented 1 year ago

Thanks for the repro! It's erroring as expected for me. Not sure what I messed up in my test app. It was previously on an older version of Glint and @types/ember* and I likely just missed something while updating.

dfreeman commented 1 year ago

We've slowly pared down the Known Limitations page as we've filled in functionality gaps and crossed off major "X flat-out doesn't work" issues, but I think both this and #575 likely belong in a "this is just something we don't have the ability to make TypeScript do" section there.