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

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

Open
wondersloth opened this issue Jun 9, 2023 · 10 comments
Labels
design Figuring out how things should work

Comments

@wondersloth
Copy link

wondersloth commented Jun 9, 2023

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
}}
  • @glint-expect-error is seen as Unused '@glint-expect-error' directive.glint
  • Linting rules in projects for these long mustache statements force formatters to wrap these arguments to new lines.

In the editor you will see the following error on someArg1

Property 'someArg1' does not exist on type '{}'.glint(2339)
  • Adding a {{! @glint-expect-error ... }} above the argument would suppress the error but not allow for valid glimmer syntax.
  • Adding the {{! @glint-expect-error ... }} comment above {{concat block and identifier still produces invalid glimmer syntax
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.

@wondersloth wondersloth changed the title @glint-expect-error cannot suppress errors from MustacheStatement in AttrNode @glint-expect-error cannot suppress errors from MustacheStatement in AttrNode Jun 9, 2023
@dfreeman dfreeman added the design Figuring out how things should work label Jun 12, 2023
@dfreeman
Copy link
Member

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:

  • 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

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

@laurmurclar
Copy link

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
Copy link
Author

wondersloth commented Jun 12, 2023

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
Copy link
Member

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
Copy link
Author

wondersloth commented Jun 12, 2023

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
Copy link
Author

wondersloth commented Jun 12, 2023

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
Copy link
Member

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:

  • as you pointed out, it's quite complicated to manage mappings between free-floating text and semantic structure in the template, and would require pretty significantly reworking (and adding nontrivial complexity to) the way that Glint manages directives today
  • even though we don't, as an implementation detail, use the corresponding @ts- directives during template emit, I believe there's meaningful value in having a 1:1 mapping of concepts, both from an ongoing maintenance perspective for Glint itself and from a pedagogical and documentation perspective for our users

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
Copy link
Author

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
Copy link
Member

👍 We'd very much welcome a code fix!

@jelhan
Copy link

jelhan commented Nov 4, 2023

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:

  • {{! @glint-ignore }} must be exactly one line above the statement to be ignored.
  • {{! prettier-ignore }} must be immediately before the node to be ignored. A comment counts as a node.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Figuring out how things should work
Projects
None yet
Development

No branches or pull requests

5 participants