Should glint prevent the "noPropertyAccessFromIndexSignature" compiler option being applied to templates?

[bwbuchanan]

@tsconfig/ember/tsconfig.json specifies "noPropertyAccessFromIndexSignature": true. And this is probably a good thing.

However, I do not think it is useful when applied to templates.

If I don't disable this check in my own tsconfig.json, I get errors reported such as:

app/components/dashboard/legend.gts:106:37 - error TS4111: Property 'label' comes from an index signature, so it must be accessed with ['label'].

106           <span class={{styles.label}}>

In this case, styles is from an imported SCSS file. The values of the record are mangled (globally unique) CSS class names generated by webpack's sass-loader / css-loader.

It's used like:

import styles from './legend.scss';

...

class Legend extends Component {
  <template>
    ...
    <span class={{styles.label}}>...
  </template>
}

Declared in global.d.ts as:

declare module '*.scss' {
  const value: Record<string, string>;
  export = value;
}

Templates accessing properties on a Record in this manner is idiomatic ember; writing it as {{get styles "label"}} is awkward.

If glint generated property accesses as styles['label'] instead of styles.label, that would resolve the errors.

Alternatives I considered:

1. Type *.scss as any instead of Record<string, string> - loses type information
2. Define a helper that is called to produce the class name, instead of an explicit lookup on the styles record, e.g. {{style "label"}} instead of {{styles.label}} - adds boilerplate and runtime overhead
3. Use {{get}} - verbose and non-idiomatic

[boris-petrov]

This is the same issue/request as I had in #196. I agree with everything written and the reasoning behind it.

[bwbuchanan]

The TypeScript compiler error message is also misleading/confusing, because you cannot write it in handlebars as {{styles['label']}} as the error message tells you to.

[dfreeman]

@tsconfig/ember/tsconfig.json specifies "noPropertyAccessFromIndexSignature": true. And this is probably a good thing.

<personal-opinion of="dfreeman">

I don't think that flag should be enabled by default—it's the only one we set in @tsconfig/ember that's strictly a stylistic lint rule and has no bearing on correctness. Whether you type foo.bar or foo['bar'] doesn't change the type of the resulting value, so it seems silly (to me!) that one should produce a type error when the other doesn't.

That kind of check seems much more appropriate in my opinion as a type-aware lint rule.

</personal-opinion>

However, I do not think it is useful when applied to templates.

I don't see that it's any more or less useful applied to templates than it is to scripts.

Templates accessing properties on a Record in this manner is idiomatic ember

Accessing fields with . notation on a Record is also idiomatic JS/TS 🙂
But setting noPropertyAccessFromIndexSignature explicitly asks the typechecker not to let you do that.

The TypeScript compiler error message is also misleading/confusing, because you cannot write it in handlebars as {{styles['label']}} as the error message tells you to.

That's absolutely a bug we should fix—thank you for calling it out!

[chriskrycho]

I don't think that flag should be enabled by default—it's the only one we set in @tsconfig/ember that's strictly a stylistic lint rule and has no bearing on correctness.

For what it's worth, my own personal opinion matches; if I recall our discussion at the time we enabled it, the problem was that if you don't have it on in the source types you can end up with things that don't work for consumers. However, that could well be mistaken—either in the assertion itself (I’m not sure how that would be true?) or in my memory!

If the assertion that it can make things appear safe in the source types that consumers cannot is wrong, though—and I think it is?—, we should loosen it back up in the default, though.

[bwbuchanan]

From a quick read of the docs, it does look like noPropertyAccessFromIndexSignature should not cause issues when used by a consumer but not by the type provider. I don't think it changes typedef semantics at all.

I tried this out:

const zz: Record<string, string> = {};

const x = zz.b; // Error: Property 'b' comes from an index signature, so it must be accessed with ['b'].ts(4111)
const y = zz['b']; // OK

type X = typeof zz.b; // OK (X = string | undefined)

From the docs:

This setting ensures consistency between accessing a field via the “dot” (obj.key) syntax, and “indexed” (obj["key"]) and the way which the property is declared in the type.
...
The goal of this flag is to signal intent in your calling syntax about how certain you are this property exists.
(emphasis mine)

So it's pretty clear that this is purely a linter flag that does not affect correctness of the program. It's probably most useful in projects that are not using noUncheckedIndexedAccess.

This is in contrast with the flags exactOptionalPropertyTypes and noImplicitOverride, which will break downstream consumers that enable them, if not conformed-to by the type provider. These flags are not currently set in @tsconfig/ember, but really should be. I've had to apply local type overrides because my app's tsconfig enables these options and multiple addons ship types that don't compile under those flags.

I now agree with @dfreeman and @chriskrycho that the default tsconfig should not set noPropertyAccessFromIndexSignature, and in the end, I don't have a strong opinion about whether glint needs an option to emit a?.['b']?.['c'] instead of a?.b?.c or if it should do that by default.

If it's a really common case that there are projects that use e.g. ember-m3 and also really want to enforce noPropertyAccessFromIndexSignature then it's something to consider. Or maybe typescript-eslint can be made to enforce this on the TypeScript code only?

For my own use case, I'll go with the option of implementing a trivial {{localClass}} helper for CSS modules:

import styles from './styles.scss';

const localClass = (klass: string) => this.styles[klass];

<template>
  <div class={{localClass "widget-base"}}>
    ...
  </div>
</template>

I'll open a separate issue regarding the need to fix the error message so that it suggests {{get abc "def"}} instead of abc['def'].

[bwbuchanan]

Ah, one other thing to consider:

glint converts property accesses of the form {{abc.foo-bar}} into abc["foo-bar"], but {{abc.foo}} into abc.foo. This means that the former will pass the noPropertyAccessFromIndexSignature check but the latter will not. This is inconsistent and perhaps an argument for converting all property accesses to the bracket form.

[dfreeman]

if I recall our discussion at the time we enabled it, the problem was that if you don't have it on in the source types you can end up with things that don't work for consumers. However, that could well be mistaken—either in the assertion itself (I’m not sure how that would be true?) or in my memory!

If the assertion that it can make things appear safe in the source types that consumers cannot is wrong, though—and I think it is?—, we should loosen it back up in the default, though.

@chriskrycho I do remember a conversation like that, but it might have been about noUncheckedIndexAccess instead? There was definitely a period where I think we all couldn't keep those two flags (and their relation to one another) straight... 😅

I think noUncheckedIndexAccess is somewhat like exactOptionalPropertyTypes: a mismatch in whether you have it enabled for your source versus whether your consumer has it enabled can't cause you to emit .d.ts files that produce errors or declarations that flat-out lie to your consumers (c.f. strictNullChecks: false). But they can make it easier to accidentally design your types in a way that's unergonomic to consume for consumers with those flags enabled.

For noPropertyAccessFromIndexSignature, though, since it's only a syntactic check, I don't think it even falls into that "could encourage bad type DX" bucket. The worst case I can think of would be providing example snippets e.g. in your README that use . notation for index properties, and TS wouldn't catch that anyway. There could be a case that I'm not thinking of, though!

This is in contrast with the flags exactOptionalPropertyTypes and noImplicitOverride, which will break downstream consumers that enable them, if not conformed-to by the type provider. These flags are not currently set in @tsconfig/ember, but really should be. I've had to apply local type overrides because my app's tsconfig enables these options and multiple addons ship types that don't compile under those flags.

@bwbuchanan can you give examples of cases where you've had to override types because the upstream definitions didn't have those flags enabled? Off the top of my head I can't construct an example of a .d.ts file that would produce errors based on a mismatch there.

[bwbuchanan]

@bwbuchanan can you give examples of cases where you've had to override types because the upstream definitions didn't have those flags enabled? Off the top of my head I can't construct an example of a .d.ts file that would produce errors based on a mismatch there.

cibernox/ember-basic-dropdown#687 was a good example, although the bug in ember-power-select that had it importing from ember-basic-dropdown's source files instead of its declarations has been fixed (ref #507)

Maybe noImplicitOverride isn't an issue if the consuming app has "skipLibCheck": true in tsconfig, but otherwise I think this .d.ts would break:

class Superclass {
  foobar(): string;
}

class Subclass extends Superclass {
  foobar(abc: number): string;  // Should be: override foobar(abc: number): string;
}

One place that exactOptionalPropertyTypes creates issues is with component args, especially if you are trying to wrap and extend a component. In essentially every case, a component is happy to accept undefined as a value for an optional arg, even if this isn't explicitly declared in the signature.

So imagine a component defined in an addon (not built with exactOptionalPropertyTypes):

interface NamedArgs {
  foo: string;
  bar?: string;
}

class UnintentionallyPickyComponent<{ Args: NamedArgs }> { ... }

Then with exactOptionalPropertyTypes enabled in your app, you can't do something like:

class Wrapper extends Component<{ Args: { foo: string; bar?: string | undefined; } }> {
  <template>
    <div class="wrapping-component">
      ...
      <UnintentionallyPickyComponent @foo={{@foo}} @bar={{@bar}}>
    </div>
  </template>
}

TypeScript will error because UnintentionallyPickyComponent will not accept undefined as a valid value for the @bar argument, even though the addon author almost certainly intended for this to be the case, and has to account for it in any event because they did not enable exactOptionalPropertyTypes.

Unfortunately, TypeScript has no way for a .d.ts file to declare "I was built without exactOptionalPropertyTypes, so just assume anywhere that I said { a?: X }, I actually meant { a?: X | undefined }."

Wrapper-style composition of components is quite common, I think. It's a shame that there's no splattributes syntax for component args.

[dfreeman]

Maybe noImplicitOverride isn't an issue if the consuming app has "skipLibCheck": true in tsconfig, but otherwise I think this .d.ts would break:

noImplicitOverride is never enforced in .d.ts files regardless of skipLibCheck.

In essentially every case, a component is happy to accept undefined as a value for an optional arg, even if this isn't explicitly declared in the signature.

The same "explicitly passing undefined is very nearly always fine" argument also applies to most TS usage, and is the reason I don't use exactOptionalPropertyTypes in most personal projects.

Wrapper-style composition of components is quite common, I think. It's a shame that there's no splattributes syntax for component args.

I agree, but/and fundamentally I think most of your argument is best served by introducing "splarguments" in Ember rather than enforcing type rules differently in TS than in templates.

I'm going to close this issue out, since the actionable technical change (fixing the error message not to suggest foo['bar'] in templates) has been done. Please feel free to open a thread in Discussions if you want to continue the discussion and see about broader community consensus on the topic 🙂