typed-ember / glint

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

`@glint-expect-error` cannot suppress errors from MustacheStatement in AttrNode #589

Open wondersloth opened 1 year ago

wondersloth commented 1 year ago

Problem

Given a template that doesn't have a Component Signature.

<div
  class="button-border-style some-style-1 some-style-2 some-style-3 some-style-4 some-style-5 some-style-6
    {{concat
      @someArg1
      @someArg2
      @someArg3
      @someArg4
      @someArg5
      @someArg6
      @someArg7
      @someArg8
      @someArg9
      @someArg10
    }}"
></div>

OR

{{! @glint-expect-error}}
{{concat
  "foo"
  @someArg2
  @someArg3
  @someArg4
  @someArg5
  @someArg6
  @someArg7
  @someArg8
  @someArg9
  @someArg10
}}

In the editor you will see the following error on someArg1

Property 'someArg1' does not exist on type '{}'.glint(2339)
Using a Handlebars comment when in the `attributeValueDoubleQuoted` state is not supported: 

|
|  {{! @glint-expect-error ... }}
|

(error occurred in 'an unknown module' @ line 3 : column 4)glint

Feature Request

A way to suppress an glint error in this case.

dfreeman commented 1 year ago

Like the @ts- directives they're based on, @glint- directives are line-based rather than tree-based. This means that Glimmer's syntax leaves us somewhat hamstrung in regions where it disallows comments. If you were to bring the whole attribute up on to one line, i.e. class="... {{concat ...}}", you'd be able to place a directive above that and have it take effect, but of course it's not ideal to force worse formatting just to suppress an error.

Glint operates against three relevant constraints here:

To move forward I think we'd need a concrete proposal for how to address this given those constraints

laurmurclar commented 1 year ago

Came here to open a similar issue for multi-line error expectations and stumbled across this. It's probably worth re-naming this to reflect the root issue?

I'm not sure about the underlying restrictions but perhaps a block-based solution could work? eg. a @glint-expect-error-start and @glint-expect-error-end directive placed above and below the error. Even being able to simply disable Glint for a block would be better than disabling it for a whole file.

{{! @glint-expect-error-start }}
<div
  class="...
    {{concat
      @someArg1
      @someArg2
      @someArg3
      @someArg4
      @someArg5
      @someArg6
      @someArg7
      @someArg8
      @someArg9
      @someArg10
    }}"
>
{{! @glint-expect-error-end }}
</div>
wondersloth commented 1 year ago

Thank you for the quick response:

Agreed, pulling up the statement up to the same line as the attribute could work, this would require changing some formatters to do this. Not great, but a potential solution.

We can have a declaration that disables the next element/block etc.

{{! @glint-expect-error-next-element: this is fine }}

<div>
  {{! @glint-expect-error-next-element: this is fine }}
  <span class="some-style
    {{concat @arg1 "foo" "bar" "baz"}}
    {{concat @arg1 "foo" "bar" "baz"}}
    {{concat @arg1 "foo" "bar" "baz"}}
    " id="end">
  </span>
</div>
chriskrycho commented 1 year ago

While I see the issue you all are trying to solve here, I would humbly suggest that the right answer is probably… add a signature instead? There are sharply diminishing returns on trying to add capabilities to Glint which increase the complexity of our mapping to TypeScript syntax, and at first blush this looks to me to be squarely in that bucket.

wondersloth commented 1 year ago

Glint operates against three relevant constraints here:

  • we can't add new syntax
  • we can't add new runtime constructs, since we have no presence at runtime
  • we can't add new "macros" or build-time constructs, since we have no presence in the compilation process

I am proposing adding adding a new DeclarativeKind @glint-expect-error-next-element would not be adding or modifying glimmer syntax.

This would be similar to glint-expect-error except rather than bounding to the next line it would wrap the next element in int he AST.

This would only affect glint runtime and evaluation of nodes. It would create a larger region wrapping the next element proceeding the MustacheCommentStatement with a directive for suppression of an error.

wondersloth commented 1 year ago

Is there some rules around a template being transformed to typescript?

Looking at the output tested in the debug.test.ts

 // @glint-expect-error\\\\n    χ.emitContent(χ.resolveOrReturn(𝚪.args.bar)());

If it were the next-element:

        | Mapping: TemplateEmbedding
        |  hbs(0:255):   {{#each (array \\"world\\" \\"planet\\" \\"universe\\") as |target index|}}\\\\n  #{{add index 1}}: {{this.message}}, {{target}}!\\\\n{{/each}}\\\\n\\\\n{{! @glint-expect-error-next-statement: no @someArg arg }}\\\\n<div class=\\"some-style\\\\n  {{concat @someArg \\"foo\\" \\"bar\\" \\"baz\\"}}\\">\\\\n</div>
        |  ts(131:902):  ({} as typeof import(\\"@glint/environment-ember-loose/-private/dsl\\")).templateForBackingValue(this, function(𝚪, χ: typeof import(\\"@glint/environment-ember-loose/-private/dsl\\")) {\\\\n  {\\\\n    const 𝛄 = χ.emitComponent(χ.resolve(χ.Globals[\\"each\\"])([\\"world\\", \\"planet\\", \\"universe\\"]));\\\\n    {\\\\n      const [target, index] = 𝛄.blockParams[\\"default\\"];\\\\n      χ.emitContent(χ.resolve(χ.Globals[\\"add\\"])(index, 1));\\\\n      χ.emitContent(χ.resolveOrReturn(𝚪.this.message)());\\\\n      χ.emitContent(χ.resolveOrReturn(target)());\\\\n    }\\\\n    χ.Globals[\\"each\\"];\\\\n  }\\\\n  // @glint-expect-error-next-statement\\\\n  {\\\\n    const 𝛄 = χ.emitElement(\\"div\\");\\\\n    χ.applyAttributes(𝛄.element, {\\\\n      class: \`\${χ.resolve(χ.Globals[\\"concat\\"])(𝚪.args.someArg, \\"foo\\", \\"bar\\", 

I explored trying to do this with finding the next mustache statements, but we would still have a mapping problem between the two formats.

To my understanding the directive maps to typescript's expect-error on the next line. So unless there is an equivalent TS feature we can't do this.

dfreeman commented 1 year ago

Our directives system isn't built directly on top of TypeScript's, so it's not entirely impossible to create new ones with their own semantics, but I'm pretty hesitant to do so for a couple of reasons:

The root limitation that I'd love to see lifted here would be for Glimmer to allow comments in a richer variety of locations in source. Short of that, I'm not sure it's going to be super viable for us to support a level of granularity in between single-line glint-ignore/glint-expect-error and whole-template glint-nocheck.

In cases where we've run into this at Salsify, we've primarily endeavored to just add signatures as @chriskrycho suggested to resolve the errors rather than masking them. When that wasn't possible, though (e.g. we had backwards compatibility code in templates for legacy args that we didn't want to include in the signature), we restructured the template instead, either by formatting things onto a single line or doing something like extracting the arg reference via {{#let @badArg as |badArg|}} where it was easy to target with a directive.

wondersloth commented 1 year ago

The root limitation that I'd love to see lifted here would be for Glimmer to allow comments in a richer variety of locations in source. Short of that, I'm not sure it's going to be super viable for us to support a level of granularity in between single-line glint-ignore/glint-expect-error and whole-template glint-nocheck.

👍 I totally agree.

@chriskrycho @dfreeman Thank you for the recommendations.

In general adding ComponentSignatures solves all the @arg problems. I think a codefix would be beneficial to help with this.

chriskrycho commented 1 year ago

:+1: We'd very much welcome a code fix!

jelhan commented 11 months ago

I struggled a lot with this limitation when using Prettier, which was rewriting the code to multiple lines. Using {{! prettier-ignore }} and {{! @glint-ignore }} together as a workaround is also not straight forward. It only works if written exactly like this:

{{! @glint-ignore }}{{! prettier-ignore }}
{{a-mustache-statement-having-arguments-which-needs-to-be-ignored-but-rewritten-by-prettier-to-multiple-lines foo="1" bar="2" baz="3"}}

Requirements:

Both together mean that {{! @glint-ignore }} must be before {{! prettier-ignore }} but on the same line. There must not be a whitespace between both as Prettier would rewrite the comments otherwise on multiple lines.

I fear this is fragile. I don't think it's a design decision that Prettier keeps two comments on the same line if there isn't a whitespace between them. I assume that's only an unintended side effect of handling {{foo}}{{bar}}. Let's hope that no one will fix it until we have a better workaround or even a solution to this limitation.