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

Support type-narrowing via discriminated unions #221

Closed
boris-petrov opened this issue Sep 28, 2021 · 14 comments · Fixed by #456 or #510
Closed

Support type-narrowing via discriminated unions #221

boris-petrov opened this issue Sep 28, 2021 · 14 comments · Fixed by #456 or #510
Labels
enhancement New feature or request

Comments

@boris-petrov
Copy link
Contributor

Type-narrowing via discriminated unions is a useful TypeScript feature. I believe it's not supported via Glint:

interface Circle {
  kind: "circle";
  radius: number;
}

interface Square {
  kind: "square";
  sideLength: number;
}

type Shape = Circle | Square;

export default class Foo extends Component<...> {
  public declare shape: Shape;

  public get foo(): number {
    return this.shape.kind === 'circle' ? this.shape.radius : this.shape.sideLength;
  }
}
{{#if (eq this.shape.kind "circle")}}
  {{log this.shape.radius}}
{{/if}}

The foo getter compiles just fine as the TypeScript compiler does it's job, however the log in the HBS gives an error:

error TS2339: Property 'radius' does not exist on type 'Shape'.
  Property 'radius' does not exist on type 'Square'.

6   {{log this.shape.radius}}
                     ~~~~~~

It would be nice to be able to do this. Not sure it's possible though. At the very least a "built-in" eq helper must be defined and Glint should be hard-coded to know about it I guess.

@dfreeman
Copy link
Member

dfreeman commented Nov 5, 2021

This is dependent the Equality Operators RFC actually landing. Until then we can't assume anything about what eq means in a template, so this kind of type narrowing isn't possible.

@bwbuchanan
Copy link
Contributor

It's been more than a year since this feature request was opened and nearly 3(!) years since the equality operators RFC was first posted.

Is there some way we can get this as an opt-in, i.e. assert to glint that the eq helper is defined as === ?

Not having the ability to discriminate unions in templates leaves the choice of either (a) adding a ton of boilerplate in the component backing class or controller to deal with all the various cases, or (b) just telling glint to ignore many templates and hoping for the best.

@dfreeman
Copy link
Member

dfreeman commented Nov 2, 2022

It's been more than a year since this feature request was opened and nearly 3(!) years since the equality operators RFC was first posted.

And the RFC hasn't been implemented in that time, so my comment above still holds 🙂
I'd love to see built-in equality operators land in Ember, and I'm as impatient as I expect you are for that to happen!

Is there some way we can get this as an opt-in, i.e. assert to glint that the eq helper is defined as === ?

Glint's maintainers have limited capacity, and we need to carefully prioritize when considering what additions and improvements we spent that capacity on. Since the RFC hasn't been implemented, other issues have taken higher priority for us, and will likely continue to for some time given our constraints. If the equality operators were to land in an Ember release, though, that would certainly change the calculus.

In the meantime, I'd be happy to review a PR contributing the ability to opt in to native semantics for equality operators; it just isn't something we have the capacity to do ourselves at the expense of other work at present.

@bwbuchanan
Copy link
Contributor

@dfreeman glad to hear that you're open to having an opt-in to the new semantics

I found it pretty easy to modify template-to-typescript.ts to add this functionality, but I'm unsure about the best way to implement a configuration option for adding eq and neq to INLINE_KEYWORDS.

Do you have any guidance or opinions on this? Thanks!

@dfreeman
Copy link
Member

@bwbuchanan I'm currently overhauling how we handle special forms like hash and yield internally to address #374 and some of the points raised in #411, and I think that should make setting up the equality and comparison operators much easier! Stay tuned once that lands, hopefully in the next day or two 🙂

@bwbuchanan
Copy link
Contributor

@bwbuchanan I'm currently overhauling how we handle special forms like hash and yield internally to address #374 and some of the points raised in #411, and I think that should make setting up the equality and comparison operators much easier! Stay tuned once that lands, hopefully in the next day or two 🙂

Nice, I see you just merged #453 . I will have a go at rebasing my changes and making up a PR.

@bwbuchanan
Copy link
Contributor

@dfreeman What do you think is right for the option name? includeEqualityOperators? allowEqualityOperators?

Or would it be better to have something like

{
  includeOptionalHelpers: ['equalityOperators', 'logicalOperators', 'numericComparisonOperators']
}

@bwbuchanan
Copy link
Contributor

Are {{eq}} and {{neq}} required to be imported from @ember/helper in strict mode or are they built-in globals?

RFC 560 is not clear about this (though it describes the helpers as "built-in") but RFC 496 would seem to suggest that anything not explicitly on its list of built-ins must be imported.

CC @chriskrycho @cibernox

@dfreeman
Copy link
Member

I think what I'd suggest instead is directly accepting special forms config in a similar way to how the GlimmerX environment accepts an additionalGlobals option.

Then users could configure their environment to opt into the specific special forms they want to enable, e.g.:

"glint": {
  "environment": {
    "ember-loose" {
      "additionalSpecialForms": {
        "globals" {
          "eq": "===",
          "not-eq": "!==",
          "and": "&&",
          "or": "||",
          // ...
        }
      }
    },
    "ember-template-imports": {
      "additionalSpecialForms": {
        "imports": {
          "ember-truth-helpers/helpers/eq": { "default": "===" },
          "ember-truth-helpers/helpers/not-eq": { "default": "!==" },
          // ...
        }
      }
    }
  }
}

We can of course bikeshed on things like the appropriate names for these new special forms, and whether ember-loose needs that intermediary globals key since all its special forms are global, but overall this approach would give us similar flexibility to additionalGlobals to account for ways that users may customize their template environment either with polyfills or their own custom preprocessing.

It's a bit more boilerplate than a simple flag would be, but I think that's ok for opting into an unstable feature of the platform, particularly given that things like import paths for strict mode haven't been established yet.

@dfreeman
Copy link
Member

Ah, I see you left a comment about import paths while I was writing that up—see above for how I think we might sidestep that particular issue within Glint 😄

@dfreeman dfreeman added the enhancement New feature or request label Nov 11, 2022
bwbuchanan added a commit to bwbuchanan/glint that referenced this issue Nov 11, 2022
Resolves typed-ember#221

As these helpers have not yet been merged into Ember, you can opt in
by adding to your glint config:

"glint": {
  "environment": {
    "ember-loose": {
      "additionalSpecialForms": {
        "globals": {
          "eq": "===",
          "not-eq": "!=="
        }
      }
    },
    "ember-template-imports": {
      "additionalSpecialForms": {
        "imports": {
          "ember-truth-helpers/helpers/eq": { "default": "===" },
          "ember-truth-helpers/helpers/not-eq": { "default": "!==" }
        }
      }
    }
  }
}
@bwbuchanan
Copy link
Contributor

PR filed! Once this is good enough to merge, I'll follow up with one to support logical and numeric comparison operators from RFCs 561 and 562.

@dfreeman
Copy link
Member

Great, thank you! I'll aim to give it a look this weekend, or Monday at the latest.

@dfreeman
Copy link
Member

I'm going to reopen this for the time being since there are still some additional operators to land. Thank you for driving this forward @bwbuchanan!

@dfreeman dfreeman reopened this Nov 16, 2022
bwbuchanan added a commit to bwbuchanan/glint that referenced this issue Jan 2, 2023
Resolves typed-ember#221

As these helpers have not yet been merged into Ember, you can opt in
by adding to your glint config:

{
  "glint": {
    "environment": {
      "ember-loose": {
        "additionalSpecialForms": {
          "globals": {
            "and": "&&",
            "or": "||",
            "not": "!"
          }
        }
      },
      "ember-template-imports": {
        "additionalSpecialForms": {
          "imports": {
            "ember-truth-helpers/helpers/and": { "default": "&&" },
            "ember-truth-helpers/helpers/or": { "default": "||" },
            "ember-truth-helpers/helpers/not": { "default": "!" }
          }
        }
      }
    }
  }
}
@bwbuchanan
Copy link
Contributor

I'm going to open a PR for logical operators (RFC 562). I believe that numeric comparison operators (RFC 561) do not require any glint special forms to implement and plain helper function typedefs should suffice.

bwbuchanan added a commit to bwbuchanan/glint that referenced this issue Jan 2, 2023
Resolves typed-ember#221

As these helpers have not yet been merged into Ember, you can opt in
by adding to your glint config:

{
  "glint": {
    "environment": {
      "ember-loose": {
        "additionalSpecialForms": {
          "globals": {
            "and": "&&",
            "or": "||",
            "not": "!"
          }
        }
      },
      "ember-template-imports": {
        "additionalSpecialForms": {
          "imports": {
            "ember-truth-helpers/helpers/and": { "default": "&&" },
            "ember-truth-helpers/helpers/or": { "default": "||" },
            "ember-truth-helpers/helpers/not": { "default": "!" }
          }
        }
      }
    }
  }
}
dfreeman added a commit that referenced this issue Jan 4, 2023
chadhietala pushed a commit to chadhietala/glint that referenced this issue Jun 15, 2023
Resolves typed-ember#221

As these helpers have not yet been merged into Ember, you can opt in
by adding to your glint config:

{
  "glint": {
    "environment": {
      "ember-loose": {
        "additionalSpecialForms": {
          "globals": {
            "and": "&&",
            "or": "||",
            "not": "!"
          }
        }
      },
      "ember-template-imports": {
        "additionalSpecialForms": {
          "imports": {
            "ember-truth-helpers/helpers/and": { "default": "&&" },
            "ember-truth-helpers/helpers/or": { "default": "||" },
            "ember-truth-helpers/helpers/not": { "default": "!" }
          }
        }
      }
    }
  }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants