-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Feat]: logical keywords #3
base: sn/comparison-operators
Are you sure you want to change the base?
Conversation
); | ||
}); | ||
|
||
function isTruthy(result: unknown) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments to respond to
I guess for now I can just say them here. Suppose someone had the following:
{{or someMaybeEmptyArray otherArray}}
If we're trying to do TS types for this we can't just convert to someMaybeEmptyArray || otherArray. As long as someMaybeEmptyArray isn't null or undefined TS will see it as being truthy and will just return the type as someMaybeEmptyArray however, the correct type would actually be someMaybeEmptyArray | otherArray. In the case where someone wants to handle empty arrays, I think it would more correctly done as:{{if someMaybeEmptyArray.length someMaybeEmptyArray otherArray}}
We could then correctly type that as someMaybeEmptyArray | otherArray.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that adopting "handlebars truthiness" for these helpers was a design mistake, and further that having handlebars truthiness differ from Javascript truthiness was itself a design mistake, though probably far too late to fix at this date.
That being said, does the typing issue actually need to be addressed internally in the glimmer VM?
Consumers will be getting their templates typechecked by glint, for which I am preparing a PR to add support for logical and equality helpers.
I think that the type for isTruthy (as glint would need to supply in its temporary translated TypeScript code) is something along the lines of:
function isTruthy(v: true): true;
function isTruthy(v: false): false;
function isTruthy<A>(v: A[]): v is [A, ...A[]];
function isTruthy<A>(v: readonly A[]): v is readonly [A, ...A[]];
function isTruthy(v: unknown): v is true | string | number | object;
function isTruthy(v: unknown): boolean {
return !!v && !isEmptyArray(v);
}
It seems that the TypeScript compiler (as of 4.8.4) is not smart enough to propagate type assertions through a function of this type, however, so to support discriminated unions, glint needs to translate an expression of the form
{{or a b c}}
into
(a && isTruthy(a)) ? a : (b && isTruthy(b)) ? b : c
which means that you can write things of the form:
type X =
| { type: 'a'; value: number }
| { type: 'b'; value: number }
| { type: 'c'; error: Error };
let foo: X;
{{#if (or (eq foo.type 'a') (eq foo.type 'b'))}}
{{log foo.value}}
{{else}}
{{log foo.error.message}}
{{/if}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm not sure that the TypeScript compiler is quite smart enough to treat something like (foo.type === 'a' && isTruthy(foo.type === 'a')) ? foo.type === 'a' : false
as equivalent to foo.type === 'a'
for type-discrimination purposes, but there is probably a way to translate the expressions that adequately expresses the semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this discussion at first, but it's definitely the crux of the issue. If we can't generate useful types from this the benefit is significantly reduced. I'll see if I can poke at this a bit to figure out what'a possible here. No RFC is ever set in stone, but at the same time, diverging from ember-truth-helpers isn't ideal either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure there's a way to capture the HBS semantics in a way that TS will propagate guards while also making the fallthrough typing {{or a b}}
correctly give typeof a | typeof b
as its type without polluting that type with | false
or similar.
I think even if this lands with the traditional HBS truthiness semantics, I'd still argue for treating them for typing purposes as though they were the native TS operators. I'm not set in stone on that (and I think there's a decent chance @chriskrycho will feel differently 😄), but to me the gains from having logical and comparison operators "just work" for type narrowing outweigh the hazard of us typing {{or stringArray numberArray}}
incorrectly as Array<string>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm putting this on the agenda for the Framework Core team meeting. The whole point of RFC Stages is that this kind of thing is useful to feed back in. I don’t want to reopen/relitigate the whole thing (YIKES) but I do want to make sure we are making a conscious and explicit choice to rule out having the ability for TS to provide both useful and accurate semantics for these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a world where there was an opt-in to Javascript truthiness semantics for templates, enabling accurate expression types for {{if}}, {{and}}, {{or}}, etc., I would 100% take that for my apps.
Having to use {{#unless (is-empty array)}}
in a few places is a small price to pay, and makes the code more explicit/scannable anyway.
Heck, if there was a strict-boolean-expressions lint for templates, I'd enforce that in my projects as well. Truthiness expressions are code landmines.
Having a semantic mismatch between Handlebars and Javascript is painful, and when you add in TypeScript, if there isn't a way to 1:1 express the semantics in the typing, you're looking at the situation where you either have something that satisfies the types but fails at runtime, or something that has to be worked around to get it to satisfy typechecking, or both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having said all that, if {{and a b c}}
is translated for typechecking purposes as a && b && c
, then at least we can use it for most type guards.
With the present Handlbars truthiness semantics, the type of the expression {{get (or a b c) 0}}
where a
and b
are Array<T>
and c
is a NonEmptyArray<T>
would ideally be T
, not T | undefined
, but this is (hopefully?) going to rarely be expressed in a template, anyway, and if you really had to write that, you'd use {{if}}
and an is-empty
helper that provides a type guard.
Is there a case where a && b && c
or a || b || c
produces an unsound type when that the helper uses Handlebars truthiness, i.e. the code passes typechecking but will fail at runtime? Having thought about this for a bit, I'm not sure that can actually happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I have strings: string[]
and numbers: number[]
, then:
{{#each (or strings numbers) as |el|}}
{{! `el` is `string` here according to TS's `||`, but could be `number` if `strings` is empty given HBS truthiness }}
{{/each}}
To be clear, even if HBS truthiness stays as it is and that means we get the wrong answer from TS in a case like ☝️, I still think the wins we get from using native operators for type analysis (which there are a lot of) are worth it.
678580a
to
6de148b
Compare
8d9dd4c
to
6348a50
Compare
86064cb
to
0ddc809
Compare
6348a50
to
4858906
Compare
0ddc809
to
1e404cd
Compare
4858906
to
1f56065
Compare
1e404cd
to
8a576d8
Compare
1f56065
to
d63c2ac
Compare
8a576d8
to
83b6e67
Compare
d63c2ac
to
01b65f4
Compare
83b6e67
to
d116bea
Compare
01b65f4
to
6e75e77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im excited about this, though I'll leave it to someone with more context to give approval.
and
or
emberjs/rfcs#562