zed icon indicating copy to clipboard operation
zed copied to clipboard

Zed forces Prettier to opt out of its usual parser inference

Open billyjanitsch opened this issue 1 year ago • 5 comments

Check for existing issues

  • [X] Completed

Describe the bug / provide steps to reproduce it

When formatting a file using its native Prettier integration, Zed sends a parser option to Prettier based on Zed's understanding of the file's language. Prettier usually infers which parser to use based on the file path, but Zed's behavior causes Prettier to opt out of this inference.

I don't think this is desirable behavior, because it can cause Zed to format files differently than Prettier would when invoked in other ways (e.g., via CLI).

One example is that Prettier special-cases package.json files, formatting them using the json-stringify parser rather than the json parser. But Zed overrides this behavior by forcing Prettier to use the json parser even for package.json files:

stderr: Resolved config: {[...]}, will format file '[...]/package.json' with options: {[...],"parser":"json","filepath":"[...]/package.json"}

This means that Zed's Prettier integration formats package.json differently than Prettier does when invoked via CLI, causing a mismatch whenever a package.json file is edited with Zed.


This could be fixed by teaching Zed about all of Prettier's various special cases, but that seems like a continuous maintenance burden that Zed probably doesn't want to take on.

Alternatively, maybe Zed doesn't need to pass the parser option at all. I think this was necessary prior to https://github.com/zed-industries/zed/pull/9598, because there was a bug in the integration that prevented the file path from being passed along, preventing parser inference. But with that bug fixed, Prettier should be able to decide on the right parser to use from the file path alone.

Environment

Zed: v0.133.7 (Zed) OS: macOS 14.4.1 Memory: 96 GiB Architecture: aarch64

If applicable, add mockups / screenshots to help explain present your vision of the feature

No response

If applicable, attach your ~/Library/Logs/Zed/Zed.log file to this issue.

No response

billyjanitsch avatar May 07 '24 20:05 billyjanitsch

Thank you for noticing.

Changing this indeed cleans up the code and plugin configs: https://github.com/zed-industries/zed/pull/11558 but has one side-effect: now, there's no opt-into prettier setting for any languages, and with the default, auto, formatting settings, all languages will send prettier formatting requests, get the failure and only then get back to the LSP requests. Previous code was able to skip prettier requests entirely.

So I wonder, should we still keep some language plugin config for explicing prettier opt-in? Or, rather, rework the autoformatting defaults and turn them off entirely?

cc @mrnugget as you were touching prettier integration recently too. I think it's fine for now the way as in the PR, but let me know your concerns.

SomeoneToIgnore avatar May 08 '24 14:05 SomeoneToIgnore

Thanks for tagging, @SomeoneToIgnore!

Few thoughts:

  1. I think the analysis is correct and that we should switch to auto, now that we pass the file name along properly.
  2. Does it make sense to add prettier_parse_name to the language settings so that users can overwrite it? I don't think it does, because I bet that setting is often file specific. And I also think you can overwrite it in prettier config too, right? Just throwing this out there.
  3. I think yes, @SomeoneToIgnore, we should make it explicit. How about we make it a boolean instead of the parser name? format_with_prettier or something like that.

mrnugget avatar May 10 '24 08:05 mrnugget

Does it make sense to add prettier_parse_name to the language settings

Good point, will get rid of it and replace with some boolean. But now, the whole "migration & backwards compatibility" story arises due to that, so I'll have to double check how things are done properly in the plugin realm.

SomeoneToIgnore avatar May 10 '24 08:05 SomeoneToIgnore

Thanks for tagging, @SomeoneToIgnore!

Few thoughts:

  1. I think the analysis is correct and that we should switch to auto, now that we pass the file name along properly.
  2. Does it make sense to add prettier_parse_name to the language settings so that users can overwrite it? I don't think it does, because I bet that setting is often file specific. And I also think you can overwrite it in prettier config too, right? Just throwing this out there.
  3. I think yes, @SomeoneToIgnore, we should make it explicit. How about we make it a boolean instead of the parser name? format_with_prettier or something like that.

Does it actually belong at the language level (i.e., in the language config)?

It seems to me that some languages that may have a Prettier formatter could also have other formatters that could be used. And it seems problematic if the language itself governs whether or not Prettier should be used.

Should we make this a language setting instead so that it is user-configurable?

maxdeviant avatar May 10 '24 20:05 maxdeviant

As a source for inspiration: image

all options are disabled by default, even for Rust and are configurable through the menu.

SomeoneToIgnore avatar May 10 '24 20:05 SomeoneToIgnore