typed-ember / glint Goto Github PK

Only original source modules should be included.

In general I think error messages should be relatively easy to map from TypeScript terms to what's happening in the template, but it's quite possible there will be situations where an error message is nonobvious if you don't know how the template is being translated to TS. JSX is natively supported by the compiler and still has this problem, and even just in vanilla TypeScript authors sometimes struggle to understand what a type error is trying to tell them.

Given that we have the original template AST node available when we're emitting error messages, we may be able to eventually bake in "Hint: this may mean you forgot to do XYZ" help when certain error codes pop up on certain node types, but in the meantime we should be keeping an eye out for common situations that may not be easy to diagnose for anyone not familiar with glint internals.

There's a (very outdated) version of this in the current README, but it needs to be updated and broken out into more consumable chunks. The target audience here isn't day-to-day users of glint, but folks who may want to contribute or better understand how it works.

Looks like something changed in typescript@next over the weekend 🙃

#7 fixed auto-import merging for most cases, but in working on the Conduit port I noticed I'd still occasionally wind up with multiple import statements from the same module.

I think I only saw it happen when both the file being edited and the import target were modules with no template transformations in them.

I'm sure I'm doing something wrong, but I'm not sure what it is.

Declaration:

type AnyFunction = (...params: any) => any;

interface RenderModifierSignature<T extends AnyFunction> {
  PositionalArgs: [T, ...Parameters<T>];
  Element: HTMLElement;
}

interface RenderModifier<T extends AnyFunction = AnyFunction>
  extends Modifier<RenderModifierSignature<T>> {}

declare module '@glint/environment-ember-loose/registry' {
  export default interface Registry {
    'did-insert': RenderModifier;
  }
}

Error:

app/components/q-tip.hbs:6:7 - error TS2769: No overload matches this call.
  Overload 1 of 2, '(item: DirectInvokable<AnyFunction>): AnyFunction', gave the following error.
    Argument of type 'RenderModifier<AnyFunction>' is not assignable to parameter of type 'DirectInvokable<AnyFunction>'.
  Overload 2 of 2, '(item: new (...args: unknown[]) => Invokable<AnyFunction>): (...args: any) => any', gave the following error.
    Argument of type 'RenderModifier<AnyFunction>' is not assignable to parameter of type 'new (...args: unknown[]) => Invokable<AnyFunction>'.

6     {{did-insert this.registerIconEl}}

This commit in the reprouction repo.

As you see, the helper has one positional parameter (that is not mandatory) and any number of named parameters. But glint complains:

app/components/test-component.hbs:1:17 - error TS2322: Type 'string' is not assignable to type 'never'.

1 {{test-helper-1 foo="bar"}}
                  ~~~

When applying an auto-import action with @glint/tsserver-plugin, a new import statement is always produced, even if one for the appropriate source module already exists. This is due to the fact that imports resolve to the transformed version of a module, but the code action attempts to add an import to the original (which, in source, ends up looking the same).

There's already a (skipped) failing test in place for this:

The docs imply you can only specify one element, but Ember does allow for multiple at the root.

I need to check our code to verify that this actually behaves as expected.

app/components/git-hub/avatar.hbs:11:15 - error TS2345: Argument of type '() => void' is not assignable to parameter of type 'string | number | boolean | void | AcceptsBlocks<{}> | null'.
  Type '() => void' is not assignable to type 'AcceptsBlocks<{}>'.
    Type 'void' is not assignable to type '{ [Blocks]: true; }'.

11       onerror={{this.imgDidError}}

    <img
      class="avatar-image"
      src={{this.avatarURL}}
      alt={{this.altText}}
      onerror={{this.imgDidError}}
    >

This commit in my reproduction repo.

As you see, I've specified PositionalArgs: [string | undefined] which should mean that a positional argument is not mandatory. But glint complains:

app/components/test-component.hbs:1:1 - error TS2554: Expected 2 arguments, but got 1.

1 {{test-helper-1 foo="bar"}}
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~

If I relocated the module utils/foo.ts to elsewhere/foo.ts and it has an import like this inside:

import { eq } from './helpers';

That import is incorrectly rewritten to:

import { eq } from '../utils./utils/helpers';

When it should be:

import { eq } from '../utils/helpers';

This might be a similar off-by-one string matching issue to the one in #12, or it might be a totally different underlying cause.

Ability to ignore lines in HBS would be great.

In https://github.com/typed-ember/glint/blob/master/packages/environment-ember-loose/types/globals.d.ts, we have the groundwork laid for specifying intrinsics available in the Ember template environment, but there are a number of items that don't yet have a type declaration, and nothing has any associated doc comments.

This should be pretty straightforward to knock out; just a matter of going through things one by one and pulling up the corresponding API docs.

Currently, all invokable template entities have a signature along the lines of (args: NamedArgs, ...positional: PositionalArgs) => Whatever. This makes named arguments first-class, and allows us to trivially represent positional arguments using the spread operator, and since the code that invokes such functions is machine-generated, adding a leading {} to the invocation when no named args are specified isn't a big deal.

However, this runs counter to @pzuraq's design for plain function-based helpers in ember-could-get-used-to-this, which bans the use of named args and only supports positional ones. We may be looking at a design clash if that's ultimately the shape things take upstream, because we can't generically represent template entities with the order swapped.

That is, it's illegal in JavaScript to have a spread anywhere other than at the end of a function's parameter list, so we couldn't use a signature like (...positional: PositionalArgs, args: NamedArgs) => Whatever.

Another alternative would be to try and remap (...positional: Positional) => T to (named: {}, ...positional: Positional) => T as part of our resolution process, but doing so would lose things like type guards, which (as I discovered here) ends up being a really important thing for helpers to be able to do once templates are typechecked.

Currently @glint/cli supports typechecking standalone template files, but the editing experience doesn't.

The first step in that direction is to update @glint/tsserver-plugin to account for standalone template files. This will ensure that e.g. private fields only consumed from templates don't show up as unused, and should allow refactorings from .ts files to be reflected in templates.

However, even once this is done, editors won't show diagnostics in template files, nor will things like quickinfo-on-hover work, since editors will have no way of knowing that tsserver can answer those questions. It seems we may need some degree of editor-specific tooling to make that work.

One option @jamescdavis and I discussed was the possibility of implementing a glint language server that could provide that information in a generic way, enabling quick integration with editors that have LSP bindings.

One thing to determine (and this may vary from editor to editor) is whether we can set that language server up to communicate with the editor's existing tsserver instance, or if the language server will need to spin up an instance of its own to service requests.

The prefix "demo" at the beginning of each adds characters and doesn't help disambiguate the folders.

This commit of my reproduction repo.

I think the component component isn't type-checked correctly at all. Pretty much all my invocations are marked as an error. As you can see in this example:

app/components/test-component.hbs:3:1 - error TS2345: Argument of type 'new () => Invokable<(args: Omit<{ index: number; }, "index"> & Partial<Pick<{ index: number; }, "index">>) => AcceptsBlocks<EmptyObject>>' is not assignable to parameter of type 'string | number | boolean | void | AcceptsBlocks<{}> | null'.
  Type 'new () => Invokable<(args: Omit<{ index: number; }, "index"> & Partial<Pick<{ index: number; }, "index">>) => AcceptsBlocks<EmptyObject>>' is not assignable to type 'AcceptsBlocks<{}>'.
    Type 'new () => Invokable<(args: Omit<{ index: number; }, "index"> & Partial<Pick<{ index: number; }, "index">>) => AcceptsBlocks<EmptyObject>>' provides no match for the signature '(blocks: {}): { [Blocks]: true; }'.

3 {{component "glimmer-component" index=1}}
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I guess that would be difficult to fix so that's one of the reasons I requested some ignore syntax.

Certain diagnostics, usually (always?) appearing on an import statement, mention the transformed name of the module rather than the original one.

If I have foo.ts open in my editor and a change on disk to bar.ts results in a new type error in foo.ts, that should be reflected in the editor, even if one or both modules in question involve template transformations.

In practice this seems to work, but currently I haven't been able to write a working test case for it. My guess this is due to tsserver being either incorrectly or incompletely configured when used in the @glint/tsserver-plugin test suite, but I'm not sure.

Figuring this out will likely involve comparing a request/response/event trace in a real editor vs in the test suite and see what's different between the two, other than the missing diagnostic.

This commit in the reproduction repo.

I want to have a helper that is able to return undefined. I believe this line prevents that:

app/helpers/test-helper-1.ts:10:10 - error TS2416: Property 'compute' in type 'TestHelper1' is not assignable to the same property in base type 'Helper<ITestHelper1>'.
  Type '([param]: [] | [string], named: { readonly [key: string]: any; }) => string | undefined' is not assignable to type '(params: [] | [string], hash: { readonly [key: string]: any; }) => string'.
    Type 'string | undefined' is not assignable to type 'string'.
      Type 'undefined' is not assignable to type 'string'.

10   public compute([param]: ITestHelper1['PositionalArgs'], named: ITestHelper1['NamedArgs']): ITestHelper1['Return'] {
            ~~~~~~~

I can run yarn glint but the linting in VSCode just seems to be stuck with outdated warnings.

It only supports a native array right now.

app/components/billing-app-usage.hbs:18:17 - error TS2345: Argument of type 'PromiseArray<{ id: string; app: App; appComponentData: { id: string; appComponent: AppComponent; totalRequests: number; }[]; totalRequests: number; percentage: number; }, Array<{ id: string; app: App; appComponentData: { ...; }[]; totalRequests: number; percentage: number; }>> | undefined' is not assignable to parameter of type 'unknown[]'.
  Type 'undefined' is not assignable to type 'unknown[]'.

18         {{#each this.appUsageData key="id" as |datum|}}

We don't need to support PromiseArray specifically, Ember.Array should be sufficient.

Currently we do very little checking of the usage of attributes and modifiers—primarily we just validate that whatever's inside the mustache is valid, but not that it is valid to apply.

There are some things we can enforce that seem uncontroversial, while others may or may not end up being a good idea.

We almost certainly want to ensure that:

• you can't pass attrs/modifiers at all to a component that doesn't have ...attributes in its template
• you can't apply a modifier that requires e.g. an HTMLAnchorElement to an HTMLDivElement (or a component with that as its root)

We might want to:

• restrict the types of values that can be set as attrs. Primitives, including functions, may be valid, but objects typically wouldn't be. Future changes to the attr-vs-property rules might impact this, but such a change was explicitly chosen not to be made as part of strict mode
• restrict the set of attributes that can be set based on the type of element they'll be applied to (i.e. no href on a <div>)
• allow component authors to artificially restrict the set of attrs they'll allow to be passed through

Cases

Forwarding:

• to an element <div ...attributes>
• to another component <Component ...attributes>

Applying attributes:

• statically to an element <div foo="bar">
• dynamically to an element <div foo={{this.bar}}>
• statically to a component <Component foo="bar">
• dynamically to an element <Component foo={{bar}}>

Applying modifiers:

• to an element <div {{someModifierThatWorksOnADiv}}>
• to a component <Component {{someModifierThatWorksOnComponentsRootElement}}>

Maybe this is a bad practice, but it does actually work despite glint being unhappy.

app/components/sortable-header.hbs:3:5 - error TS2345: Argument of type '{ query: { sortKey: string; }; }' is not assignable to parameter of type 'LinkToArgs'.
  Property 'route' is missing in type '{ query: { sortKey: string; }; }' but required in type 'LinkToArgs'.

  3     <LinkTo @query={{hash sortKey=this.sortKey}}>
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  4       <span class="sortable-header-name">
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
... 
 13       </span>
    ~~~~~~~~~~~~~
 14     </LinkTo>

#22 comprises a big chunk of the work necessary to support today's Ember idioms, but we additionally need an environment that exposes types that reflect how Ember components (and controllers!) interact with templates.

One question here is what to call this environment. My hope (and expectation) is that strict mode eventually becomes the norm and is widely adopted, as that makes everything glint aims to do much simpler. Given that, @glint/environment-ember would ideally refer to that environment, but that raises the question of what to call the environment that supports today's idioms. @glint/environment-ember-lax? @glint/environment-ember-classic? Or, if strict mode ultimately comes as part of a new edition, then maybe this is @glint/environment-octane?

Check this repo (actually this specific commit). It adds a mixin and a component that uses it.

Running ./node_modules/.bin/tsc --noEmit leads to no errors. However, running ./node_modules/.bin/glint causes:

app/components/test-component.ts:4:44 - error TS4020: 'extends' clause of exported class 'TestComponent' has or is using private name 'ICalendar'.

4 export default class TestComponent extends Component.extend(Calendar) {
                                             ~~~~~~~~~~~~~~~~~~~~~~~~~~

app/components/test-component.ts:4:44 - error TS4020: 'extends' clause of exported class 'TestComponent' has or is using private name 'ICalendar'.

4 export default class TestComponent extends Component.extend(Calendar) {
                                             ~~~~~~~~~~~~~~~~~~~~~~~~~~

There shouldn't be a difference here as there are is no HBS involved so Glint should behave exactly the same as tsc.

P.S. Exporting the ICalendar interface fixes the errors however I still believe this is a bug because of the discrepancy between the two type-checkers.

Currently template-only components are unsupported. There are a few ways we might possibly go, but in the short term the simplest path forward might be to support having a .d.ts file alongside a template that can declare its associated type(s). Longer-term, as the community adopts standards for strict-mode templates and we see how that ends up looking, we can consider ways to define the types in the same file as the template itself.

{{! app/components/my-component.hbs }}

{{@name}} said:

<blockquote>{{yield to="quote"}}</blockquote>

// app/components/my-component.d.ts

import Component from '@glimmer/component';

export interface MyComponentArgs {
  name: string;
}

export interface MyComponentYields {
  quote: [];
}

export default class MyComponent extends Component<MyComponentArgs, MyComponentYields> {}

https://github.com/emberjs/ember.js/blob/v3.25.1/packages/%40ember/-internals/glimmer/lib/components/link-to.ts#L143

app/components/nav/bar/index.hbs:16:13 - error TS2345: Argument of type '{ route: string; models: (AppComponent | DataRange | null)[]; }' is not assignable to parameter of type 'LinkToArgs'.
  Object literal may only specify known properties, but 'models' does not exist in type 'LinkToArgs'. Did you mean to write 'model'?

16             @models={{array this.currentAppComponent this.currentRange}}

As #117 uncovered, we don't currently have any test coverage for the runtime re-exported values from ember-modifier.

We should add code in test-packages/ts-ember-app that exercise those, and we should also likely audit our other GlimmerX and Ember re-exports to make sure we aren't missing coverage for anything else.

I've given the tsserver plugin a once-over in a few different editors and verified it seems to more or less work as expected, but VS Code is what I use in my day-to-day, and I really have no clue how folks who use things like Emacs, Vim or IntelliJ/WebStorm actually typically use their editors, so I don't know for sure what is or isn't working as expected.

Testing things out in a wider variety of editing environments would be fantastic.

Currently, glint supports GlimmerX-style components, where a component's template is defined using a tagged template literal inside its class body.

Regardless of the form factor strict mode and template imports ultimately take, to support today's idioms in Ember we need to be able to consume templates in standalone files. This forces the introduction of some amount of project-structure knowledge into glint, but fortunately isolating these sorts of details is exactly the kind of thing environments were introduced to handle.

We probably want to support template-only components and colocated component/template pairs as a minimum starting point. That is, if <root> refers to app or addon as appropriate, then:

• <root>/components/<name>.hbs should pair with <root>/component/<name>.ts if present
• <root>/components/<name>.hbs should be treated as a template-only component otherwise (and if no other cases described below apply)

There are a pretty large number of other cases we can hopefully eventually detect well enough to support route/controller templates and to give folks in classic/pods layouts a migration path.

• <root>/templates/components/<name>.hbs should pair with <root>/components/<name>.ts if present
• <root>/templates/<name>.hbs should pair with <root>/controllers/<name>.ts if present
• <root>/<path>/template.hbs should, in order:
  • pair with <root>/<path>/component.ts if present
  • pair with <root>/<path>/controller.ts if present
  • be treated as template-only otherwise (is /template the import path?)

Convert signatures, add to registry, etc.

To support today's Ember idioms in advance of template imports and strict mode, we need a way for glint to resolve template entities that aren't explicitly statically in scope.

The resolution process in @glint/template was designed with this in mind, and a likely approach here is to have the Ember environment we create in #23 include a publicly-extensible type registry in the set of global values it exposes to glint.

To make this less of a pain (particularly for the initial setup), it would be great if we could automate the process of generating this registry.

Currently, @glimmer/component looks something like this:

// @glimmer/component
class Component<Args> {
  readonly args: Args;
}

// my-component.ts
interface MyComponentArgs {
  foo: string;
}

class MyComponent extends Component<MyComponentArgs> {}

This RFC suggests adding a second type parameter to capture the types of a component's yields, something like:

// @glimmer/component
class Component<Args, Yields> {
  readonly args: Args;

  // Not visible anywhere from TS, but necessary to capture the type
  [YieldsBrand]: Yields;
}

// my-component.ts
interface MyComponentArgs {
  foo: string;
}

interface MyComponentYields {
  default: [message: string];
}

class MyComponent extends Component<MyComponentArgs, MyComponentYields> {}

However, this may not scale well as we consider other information we could eventually want to encode, such as #21. Capturing the type of a template's root element would add a third type param, and if we decide to allow authors to explicitly specify valid attrs, that would be a fourth.

A class with four type parameters is already unwieldy, but it's especially so if not every parameter will always be specified. An author might never yield from their component, but still have ...attributes applied to an element.

Given that, a signature along these lines may be more ergonomic (and more amenable to future expansion as necessary):

interface ComponentSignature {
  args?: object;
  blocks?: object;
  element?: Element;
}

class Component<T extends ComponentSignature> {
  readonly args: T['args'];

  // As above, not visible from TS but necessary to capture the full type
  [SignatureBrand]: T;
}

However, there are some questions we need to answer:

• Is the danger of typos an issue? ({ arrgs: MyComponentArgs } would be valid, just not useful)
• Adding type parameters is backwards compatible (as long as there's a default), but nesting component args under an args key in the type would be a breaking change for @glimmer/component—how much trouble would that cause?

This likely means that get isn't being used as it should be.

app/components/nav/menu/app-switcher.hbs:181:54 - error TS2339: Property 'repoUrl' does not exist on type 'AsyncBelongsTo<App>'.

181                 <ExternalLink href={{this.currentApp.repoUrl}} class="menu-kebab-link">View On GitHub</ExternalLink>

export default class App extends Model {
  @attr('string')
  declare repoUrl: string | null;

  // ...
}

While I'm not sure if we should be doing this, this currently does work:

export default class ImageLoader extends Component<ImageLoaderSignature> {
  @tracked protected img: HTMLImageElement | null = null;

 // ...
}

{{this.img}}

However, Glint doesn't like it:

app/components/image-loader.hbs:11:5 - error TS2345: Argument of type 'HTMLImageElement | null' is not assignable to parameter of type 'string | number | boolean | void | AcceptsBlocks<{}> | null'.
  Type 'HTMLImageElement' is not assignable to type 'string | number | boolean | void | AcceptsBlocks<{}> | null'.
    Type 'HTMLImageElement' is not assignable to type 'string'.

11     {{this.img}}

{{#async-await this.promise as |value|}}
    <SynchronousComponent @value={{value}} />
  {{else}}
    <LoadingSpinner />
  {{/async-await}}

https://github.com/tildeio/ember-async-await-helper/blob/master/addon/components/async-await.js

    {{#if this.showTip}}
      <QTip
        @description={{@description}}
        @learnMorePath={{@learnMorePath}}
      />
    {{/if}}

In the component args, description can be string | undefined, but QTip requires description to be string. However, this is acceptable since showTip will never be true if description is undefined.

I realize that TS won't know this, but some sort of assert would help here. Something like:

    {{#if this.showTip}}
      {{! glint-assert "expected description" @description}}
      <QTip
        @description={{@description}}
        @learnMorePath={{@learnMorePath}}
      />
    {{/if}}

This commit in my reproduction repo.

The built-in Input component supports passing both value and @value (as well as a number of other similar properties). Glint doesn't - when using a property without a @, the following errors is emitted:

app/components/test-component.hbs:3:1 - error TS2344: Type 'null' does not satisfy the constraint 'Element'.

3 <Input value="asd" />
  ~~~~~~~~~~~~~~~~~~~~~