Formatting breaks @ts-ignore comments

[michaeljota]

edited by @thorn0, citing this comment:

@ts-ignore ignores one line of code, not one expression or statement (a behavior diametrically opposed to code beautifiers which operate on the AST). It therefore almost always affects code where Prettier condenses or spreads out TypeScript code.

Prettier 1.19.1
Playground link

--parser typescript

Input:

// @ts-ignore
const result = evaluate(chain, opts.filename, args.map(arg => arg.node.value))

Output:

// @ts-ignore
const result = evaluate(
  chain,
  opts.filename,
  args.map(arg => arg.node.value)
);

The original code example is below. Prettier doesn't break it anymore, but the general problem with @ts-ignore comments hasn't been solved.

original code example

Prettier 1.15.1
Playground link

# Options (if any):
--single-quote

Input:

export class WebpackTranslateLoader implements TranslateLoader {
  public getTranslation<T>(lang: string): Observable<T> {
    return from(
      // @ts-ignore 
      import('dinamicModule')
    );
  }
}

Output:

export class WebpackTranslateLoader implements TranslateLoader {
  public getTranslation<T>(lang: string): Observable<T> {
    return from(
      import(// @ts-ignore
      "dinamicModule")
    );
  }
}

Expected behavior:

Same as input. The comment is mean to ignore the function call, but by moving the comment inside the function, the meaning of the comment is lost.

[loilo]

This is a really hard problem, not just a "move @ts-ignore elsewhere" issue.

@ts-ignore ignores one line of code, not one expression or statement (a behavior diametrically opposed to code beautifiers which operate on the AST). It therefore almost always affects code where Prettier condenses or spreads out TypeScript code.

Line Spreading

Below is an example from my current project (which brought me to this issue). The @ts-ignore is prepended because arg.node.value throws an error due to wrong external typing.

This is the original code:

// @ts-ignore
const result = evaluate(chain, opts.filename, args.map(arg => arg.node.value))

It becomes:

// @ts-ignore
const result = evaluate(
  chain,
  opts.filename,
  args.map(arg => arg.node.value)
)

When it should become:

const result = evaluate(
  chain,
  opts.filename,
  // @ts-ignore
  args.map(arg => arg.node.value)
)

And since there's no way for Prettier to know which is the part to be ignored, it should either bail out of formatting that line completely or prepend a @ts-ignore before every single line:

// @ts-ignore
const result = evaluate(
  // @ts-ignore
  chain,
  // @ts-ignore
  opts.filename,
  // @ts-ignore
  args.map(arg => arg.node.value)
// @ts-ignore
)

This is pragmatic yet in no way... prettier than before. Therefore I'd vote for bailing out on a @ts-ignore encounter.

Line Condensing

However, even bailing out is not always trivial. It becomes way harder when the line below the @ts-ignore is only part of an expression or block.

Take this example:

// @ts-ignore
const foo = () =>
  bar()

which is prettified to:

// @ts-ignore
const foo = () => bar();

It's not strictly breaking anything, but it might in rare edge cases suppress TypeScript errors that were not supposed to be suppressed.

---

No actual bottom line here, those are just some cases to consider when attempting to fix this issue.

[michaeljota]

@loilo

This is the original code:

// @ts-ignore
const result = evaluate(chain, opts.filename, args.map(arg => arg.node.value))

It becomes:

// @ts-ignore
const result = evaluate(
  chain,
  opts.filename,
  args.map(arg => arg.node.value)
)

When it should become:

const result = evaluate(
  chain,
  opts.filename,
  // @ts-ignore
  args.map(arg => arg.node.value)
)

I don't think this is true. Yes, the formatter would break the statement, but it should be manually fixed as there is no way for Prettier to understand that this is the desired behavior.

The issue is actually about that, instead of trying to understand what the // @ts-ignore is doing, just ignore it, and format everything around it. If, as you expose, it breaks something, I think is better that it does it by omission, than because Prettier actually tried to understand where the comment belongs.

[loilo]

That sounds reasonable. I actually considered writing down that option too but apparently somehow forgot about that, sorry.

It's probably the best approach as 100% correct behaviour seems unachievable with either solution applied, so the one wth the least disruption should be chosen.

[lvl99]

I'm gonna throw in my two cents on a // @ts-ignore use-case I'm trying to use, which prettier seems to break:

function flattenOptions(
  options: ReactSelectOption[]
): ReactSelectOptionObject[] {
  return options.reduce(
    (acc: ReactSelectOptionObject[], option: ReactSelectOption) =>
      option.hasOwnProperty("options")
        // @ts-ignore: totally legal thing to do
        ? flattenOptions(option.options)
        // @ts-ignore: another totally legal thing to do
        : acc.concat([option]),
    []
  );
}

gets formatted like so and means the // @ts-ignore lines aren't in effect:

function flattenOptions(
  options: ReactSelectOption[]
): ReactSelectOptionObject[] {
  return options.reduce(
    (acc: ReactSelectOptionObject[], option: ReactSelectOption) =>
      option.hasOwnProperty("options")
        ? // @ts-ignore: totally legal thing to do
          flattenOptions(option.options)
        : // @ts-ignore: another totally legal thing to do
          acc.concat([option]),
    []
  );
}

I've also tried using // prettier-ignore-start and // prettier-ignore-end and haven't been able to get VS Code prettier to ignore it...

[j-f1]

Does the formatted version no longer ignore the issues?

[lvl99]

@j-f1 yeah, it stops the effectiveness of the // @ts-ignore. I think // @ts-ignore lines have to occupy a single line alone. It seems only whitespace is allowed before the comment on the same line?

[lydell]

As a workaround, this might work:

Prettier 1.16.4
Playground link

--parser babylon

Input:

function flattenOptions(
  options: ReactSelectOption[]
): ReactSelectOptionObject[] {
  return options.reduce(
    (acc: ReactSelectOptionObject[], option: ReactSelectOption) =>
      option.hasOwnProperty("options")
        // dummy comment to make ts-ignore work
        // @ts-ignore: totally legal thing to do
        ? flattenOptions(option.options)
        // dummy comment to make ts-ignore work
        // @ts-ignore: another totally legal thing to do
        : acc.concat([option]),
    []
  );
}

Output:

function flattenOptions(
  options: ReactSelectOption[]
): ReactSelectOptionObject[] {
  return options.reduce(
    (acc: ReactSelectOptionObject[], option: ReactSelectOption) =>
      option.hasOwnProperty("options")
        ? // dummy comment to make ts-ignore work
          // @ts-ignore: totally legal thing to do
          flattenOptions(option.options)
        : // dummy comment to make ts-ignore work
          // @ts-ignore: another totally legal thing to do
          acc.concat([option]),
    []
  );
}

[lvl99]

Sweet trick @lydell — it works! However a part of me wishes that this could be an internal rule that prettier can automatically work around.

[duailibe]

I think // @ts-ignore lines have to occupy a single line alone

Wow that's unfortunate

[glen-84]

This seems to be fixed now?

[alexander-akait]

Yes, original issue was solved, feel free to open new issue if you have other problems with comments, thanks

[thorn0]

The specific example from the top post works now, but the general problem with // @ts-ignore has not been solved. Let's reopen this issue as it has valuable comments.

[alexander-akait]

@thorn0 I prefer new issues, #5411 (comment) should be solved manually not on prettier side, we can't duplicate comments, same problem with eslint comments and etc, we already discussed about it

[alexander-akait]

Comments inside ternary have other issue, so we can close this issue

[thorn0]

I'm not talking only about ternaries. I mean the general problem with the fact that ts-ignore is applied on the line level. The solution here might be to treat it as prettier-ignore placed in front of the whole statement.

[alexander-akait]

@thorn0 still can't understand problem and how we can solve this, please clarify, as i said early comments like eslint-ignore, ts-ignore, prettier-ignore should be solved on manual level

[thorn0]

The problem is described in the comment I linked to.

In order to not break anything,

foo(
  baz,
  // @ts-ignore
  bar
);

should be treated as

// prettier-ignore
foo(
  baz,
  // @ts-ignore
  bar
);

[thorn0]

Sorry, I deleted my previous comment. It was completely wrong. Ignoring the whole statement seems to be a solution, albeit an ugly one. So this issue is fixable.

[loilo]

Also my statement which you cited in the deleted comment is completely bogus, gonna update my comment.

This comment is completely bogus, ignore it.

[alexander-akait]

@thorn0 i disagree, we should not ignore nodes with comments, other developers can potential use other comments with any values and we can't change output based on comment text

[thorn0]

we can't change output based on comment text

We already do so with JSDoc type assertions. And // prettier-ignore itself is a comment too.

[glen-84]

That solution would also cause other parts to be ignored, so:

foo(
baz,
// @ts-ignore
bar,baz
);

... would not be formatted at all?

[thorn0]

@glen-84 Right. But what else can be done here?

[alexander-akait]

Also original issue was about other problem, if you want to discussion how we can solve post/feedback from the issue you need open an new issue, it is very hard to track problems inside thread in other issue

[alexander-akait]

We also try to ignore original input as much as possible when we format, so you change formatting based on comment text is horrible idea for me. Also it is very bad for DX, better get errors and move ts-ignore comments on other lines than get ugly code.

[thorn0]

@evilebottnawi Do you really need an example of how // prettier-ignore affects the output? :)

I wouldn't say the original issue was different. The title and the discussion of this issue are about the generic problem. I really don't see why open a new one.

Using ts-ignore is a bad idea too, so I actually somewhat like the fact that Prettier breaks it. But still, imagine a situation where Prettier runs on the pre-commit hook: the code compiles before the commit, you commit, Prettier breaks ts-ignore, now it doesn't compile for everyone...

[alexander-akait]

@thorn0 okay, please rename issue and move main idea of problem on top, otherwise better close original issue and open new, it is really hard to search problems inside posts into thread

[glen-84]

@evilebottnawi It was hypothetical.

@thorn0 Apologies, I missed that spreading case, but I still believe that:

1. Ignoring formatting on the whole statement is far from ideal.
2. A similar issue could easily occur with other tools using other comments.

I don't see how this issue could be solved.

[thorn0]

@ts-ignore is supposed to be used as a temporary measure. It's very much like a TODO comment. So fix all your @ts-ignores and now all your code is formatted again. It really looks like a good enough trade-off.

[glen-84]

Would it be possible to disable the spreading behaviour for the line following a ts-ignore comment?

In that way, at least the rest of the line is formatted correctly.

[thorn0]

No, Prettier doesn't operate on lines.

[glen-84]

The next AST node then.

[loilo]

Wouldn't be sufficient either.

// @ts-ignore
const foo = 1; const bar = 2;

[glen-84]

Who writes code like that? 😛

Also, the previously-suggested solution would have the same limitation.

I don't know anything about the internals of Prettier, but perhaps you could use the location metadata in the AST to ignore all nodes on the line following the comment.

[thorn0]

@glen-84 It's not worth the trouble. @ts-ignore is like the with statement or the comma operator. We just don't want Prettier to break them. That's it. We shouldn't really care how they look in the end. That said, if you feel like trying to implement a more sophisticated solution, I'll be glad to review the PR.

[lobsterkatie]

Just pinging here, as I just ran into exactly this problem (from earlier in this thread) with ternaries and VSCode: #5411 (comment)

The workaround with dummy comments does work, but it would be great if the underlying problem could be fixed.

NB: Someone mentioned that there is a separate issue for ternaries, but I couldn't find it. Happy to move this there if someone can point me in the right direction.

Thanks!

[wisefool769]

Alternatively, prettier could apply ts-ignore to every line when it does an expansion -- as noted by @loilo . This would be semantically identical to the original code, which would be my default expectation when running a prettifier. I will admit it is not "prettier", but despite the name of the tool I believe its primary purpose is to achieve consistency in a codebase -- not actually to monotonically make it prettier every time it is run.

In any case, ts-ignore should be used sparingly in the codebase. Its usage is fundamentally an eyesore already. If there are extraneous ts-ignore's inserted by prettier, devs can just remove them over time.

[ddubrava]

I know it's obvious, but just to mention, it's not only about @ts-ignore; the same thing goes for eslint-disable comments

[so1ve]

IMO prettier should maintain a list of "directive comments", and ignore lines below these comments.

[iteriani]

fwiw, within google we've locally patched prettier to treat ts-ignore as prettier-ignore. This seems to work alright.

[lobsterkatie]

@iteriani - Neat idea. To save some digging, mind sharing how you did that?

[iteriani]

There's some function called

function isPrettierIgnoreComment(comment) {}

We basically did something like

  return (comment.value.trim() === "prettier-ignore" || comment.value.trim().includes("@ts-ignore")) && !comment.unignore;

[lobsterkatie]

Great, will give it a shot, thanks!