jscheid / prettier.el

Prettier code formatting for Emacs.
GNU General Public License v3.0
163 stars 11 forks source link

FR: easier debugging of configuration #92

Open aspiers opened 2 years ago

aspiers commented 2 years ago

I have a repo where if I run yarn prettier -c foo.json it formats it one way, and when I save it in emacs with this prettier-mode enabled, it formats it another way - and I can't figure out for the life of me why.

I do appreciate that the prettier executable run via yarn is different to the base64-encoded one which comes with prettier.el (or at least it does in my case, when obtaining it from MELPA via straight.el, so maybe the difference is related to that, e.g. different prettier versions with different defaults? Or maybe they are somehow picking up different configurations? Although the latter seems unlikely since I only have one .prettierrc.yml at the top of the repository in question.

I've looked at the source to see if there's any way to find out the prettier config which the prettier.el long-running process is using, but there's nothing obvious. I'm not even sure whether prettier.el can simultaneously support multiple different configurations if your emacs has files from different projects open at once, each with different .prettierrc settings?

Describe the solution you'd like

Describe alternatives you've considered

I instrumented prettier--sync-config and prettier--load-config with Edebug and stepped through them, but that just showed me some hugely complex data structure returned from the server, so it wasn't any use.

jscheid commented 2 years ago

@aspiers this package derives the parser from the major mode used, while Prettier normally uses heuristics mostly, but IIRC not exclusively, based on the file name.

The idea was that Emacs might know better which parser to use, but I've come to regret that decision because it can lead to discrepancies. I'm guessing your case is a casualty.

I've been planning to rewrite that part of the code, but haven't found the time yet. Somewhat related I also want to repurpose (parts of) https://github.com/editorconfig/editorconfig-emacs for setting indentation offsets etc. I can't remember immediately why but I think that's a prerequisite.

As for your suggestion, I'm not convinced yet it's necessary. I'd like to first try to see if the removal of parser detection would help you.

jscheid commented 2 years ago

You could compare the parser used by the Prettier CLI vs that used by prettier.el:

prettier --file-info <path> | jq .inferredParser will print the parser used by the CLI. M-: (car (prettier--parsers)) RET will show the one used by prettier.el.

I'm guessing they are different, if so try overriding the parser used by prettier.el by setting variable prettier-parsers and see if that lets you work around it for now.

aspiers commented 2 years ago

Thanks a lot for the quick responses! I've just stumbled across https://github.com/prettier/prettier/issues/10876 which helped me figure out that the mismatch only occurs when the file is called package.json! So in that case, the CLI version is using json-stringify instead of json:

% prettier --file-info package.json | jq .inferredParser     
"json-stringify"
% prettier --file-info package-copy.json | jq .inferredParser
"json"

whereas M-: (car (prettier--parsers)) shows json for both.

According to that issue, this (undocumented!!) behaviour is intentional, but frustratingly, some stupid github bot has locked the issue for new comments. Any ideas how prettier.el should handle this?

BTW, the fact that prettier.el depends on the major-mode did indeed trip me up previously, since I was using js-mode (foolishly, or otherwise), and this was resulting in syntax errors from prettier.el. I've since discovered https://github.com/joshwnj/json-mode/, which is unfortunately unmaintained, but regardless seems to be the best option for editing JSON in emacs I could find. So I switched to that, and the syntax errors got replaced by this other issue described above ;-)

jscheid commented 2 years ago

Aha! I was wondering if it might be package.json. There is this: https://github.com/jscheid/prettier.el/commit/e0b234e752d67feea49da091d8c3642f108d6609#diff-0bfd05293a2b9d0104bbfe3773359ad0f4e45fd1fe2dae6c34da51612abb0525R372-R377

Would you mind adding a bit of debug info to see why it's not working as intended?

aspiers commented 2 years ago

I think I found the issue: it's that json-stringify is missing from (defcustom prettier-enabled-parsers ...).

BTW I've just filed https://github.com/prettier/prettier/issues/11553.

aspiers commented 2 years ago

More detail: M-x (prettier--parsers-for-mode 'json-mode) returns (json-stringify json json5), but M-: (car (prettier--parsers)) shows json as previously mentioned, because json-stringify gets filtered out due to not being enabled.

aspiers commented 2 years ago

M-x customize-variable shows that json5 is the only other one not enabled by default. I guess it was a simple case of forgetting to add them to the default list?

aspiers commented 2 years ago

Yep - and after enabling, everything works perfectly. Phew! Thanks a lot for your help! Will you add those two to the default list?

jscheid commented 2 years ago

Ah, good find. Yeah I think those two are just an oversight. Happy to merge a PR right away, otherwise I'll try to get to it soon.

aspiers commented 2 years ago

BTW (and sorry for the spam, hopefully this is the last message) - going back to my original suggestion of making it easier to debug where config comes from, I think at very least something like the following would be helpful (untested):

(defun prettier-current-parser ()
  "Show the prettier parser selected for the current buffer."
  (interactive)
  (message "Prettier parser for current buffer is %s" (car (prettier--parsers))))

But even though that is a nice step in the right direction, it still doesn't answer my questions about whether prettier.el can handle multiple .prettierrc configurations at the same time, and if so, which one is currently active. For example, what if you have a big monorepo with a .prettierrc at the top, and another one under a packages/foo/ yarn workspace? Look at prettier's debug output for comparison:

yarn prettier -c --loglevel debug package.json
yarn run v1.22.5
$ ./node_modules/.bin/prettier -c --loglevel debug package.json
[debug] normalized argv: {"_":["package.json"],"color":true,"editorconfig":true,"loglevel":"debug","config-precedence":"cli-override","debug-repeat":0,"ignore-path":".prettierignore","plugin":[],"plugin-search-dir":[],"check":true}
Checking formatting...
[debug] resolve config from 'package.json'
[debug] loaded options `{"useTabs":false,"tabWidth":2,"semi":true,"singleQuote":true}`
[debug] applied config-precedence (cli-override): {"semi":true,"singleQuote":true,"tabWidth":2,"useTabs":false}
All matched files use Prettier code style!
Done in 0.54s.

While this is not perfect (e.g. it doesn't mention loading preferences from .prettierrc.yml), it does give a nice amount of useful information. It would be great if for example that could be output to the *prettier (local)* buffer or similar.

jscheid commented 2 years ago

"Show the prettier parser selected for the current buffer."

Note that the default modeline ("lighter") is already showing the parser -- except that it seems to be showing the wrong parser for some reason, but I'd rather fix that than add a new function.

But even though that is a nice step in the right direction, it still doesn't answer my questions about whether prettier.el can handle multiple .prettierrc configurations at the same time, and if so, which one is currently active. For example, what if you have a big monorepo with a .prettierrc at the top, and another one under a packages/foo/ yarn workspace? Look at prettier's debug output for comparison:

Aside from the parser override, which as discussed I am already planning to remove, we're leaving all of this up to Prettier; meaning that there would be little benefit in having this package output the information if you can already ask the Prettier CLI for it.

jscheid commented 2 years ago

Note that the default modeline ("lighter") is already showing the parser -- except that it seems to be showing the wrong parser for some reason, but I'd rather fix that than add a new function.

Actually, I checked and it works as intended. Does that help at all?

aspiers commented 2 years ago

My modeline is already too crowded and I don't think I have space to keep prettier info on there, to be honest. Since the logic already exists to figure out what to display to the user, why not allow two different ways to get it depending on preferences? Adding a tiny defun like the one above seems like an easy win with no obvious downside (unless I'm missing something).

there would be little benefit in having this package output the information if you can already ask the Prettier CLI for it.

IMHO there is more than a little benefit. For example (IIUC) there could be multiple local installations of prettier (e.g. for different nvm-installed versions of node), or other things which might go wrong in the integration between prettier and the elisp. In those cases, it's really useful to get the elisp's perspective, to check whether it aligns with the underlying prettier's perspective. The problem I encountered above (where prettier inferred json-stringify, but prettier.el inferred json) is a perfect example of that: it was only after you told me the (car (prettier--parsers)) trick that I figured out the discrepancy between the two. Another scenario might be a discrepancy between which .prettierrc is being used. A third might be some discrepancy between running prettier as a one-off from the CLI vs. running it as a long-running server process. I'm sure there are other things which could go wrong too.

So my suggestion is that users should be able to easily perform some of their own debugging even before having to learn the elisp internals of prettier.el. By "easily", I mean: a) without needing to know any elisp, and b) without having to figure out the exact CLI command for running prettier in a way which accurately mirrors how prettier.el is executing it.

jscheid commented 2 years ago

My modeline is already too crowded and I don't think I have space to keep prettier info on there, to be honest. Since the logic already exists to figure out what to display to the user, why not allow two different ways to get it depending on preferences? Adding a tiny defun like the one above seems like an easy win with no obvious downside (unless I'm missing something).

Fair enough. The info you seek is in buffer-local variable prettier-last-parser -- it's the parser last used for formatting. I'll happily merge a PR that adds documentation for this or a wrapper function.

The problem I encountered above (where prettier inferred json-stringify, but prettier.el inferred json) is a perfect example of that

I'm not so sure, it's the first bug report I've had in months and I can count on one hand the number of bug reports to do with a discrepancy between CLI and package; and then it's not even clear if the other reporters would have used the feature you're proposing rather than just raise a ticket here and let me figure it out.

It took you and me all of 30 minutes to solve, and we both did something else during that time as well (in your case, file an issue with Prettier about the undocumented feature.) Maybe it would have taken less time if you had included the prettier-info dump, which would have shown that the problem occurs with package.json.

Building this feature, on the other hand, is going to take the better part of an evening and then someone will have to maintain it going forward. So I'm not convinced that this here was a good example of why we need this facility. In addition, this particular kind of problem, as well as the previous similar bug report (#88) won't happen anymore once I remove the custom parser detection.

By the way, we let Prettier determine which .prettierrc to use, via resolveConfig.

In short, I'm not particularly keen on building this feature myself at this point in time. That being said, if you wanted to build it I'd be happy to lend you a hand and merge it, if it doesn't add all too many LOC that I will have to then maintain.

aspiers commented 2 years ago

Thanks very much for the reply and sorry for the delay in getting back to you.

I've just experienced a completely new issue with prettier.el (this time with Solidity code rather than JSON), but it's put me back in the situation where I'm trying to debug the configuration. It seems relevant to post in this issue rather than a new one, because despite your very helpful tips above, I'm still unable to figure out what's going wrong. Maybe it will turn out to be a genuine bug in prettier.el rather than an issue with my configuration which needed debugging, in which case I'll happy retract the suggestion that this further reinforces the need for easier debugging of configuration. But currently it seems more likely that I'm doing something wrong than prettier.el has a bug ;-)

The problem this time is that prettier-prettify is not prettifying the buffer at all. (car (prettier--parsers)) returns 'solidity, but prettier-last-parser is "none" (buffer-local). Also this looks fine:

$ prettier --file-info /path/to/file.sol | jq .inferredParser
"solidity-parse"

Here is the censored prettier-info dump:

https://gist.github.com/aspiers/d2f801669bf144f336eac5c6c7b9043a

jscheid commented 2 years ago

@aspiers the prettier-info dump suggests that the Prettier installation being picked up does not have prettier-plugin-solidity installed.

The prettier-info dump currently doesn't include the location from where Prettier is picked up, which I'm going to improve. And I do grant you that it would probably be useful to surface in a more accessible way.

That being said, the logic is to find the closest package.json starting with the path of the file being formatted, and using the "local" Prettier installation from there if possible, falling back to the global prettier installation which is determined by trying, in this order:

  1. require("prettier")
  2. require(`${npm_root}/prettier`), where npm_root is the output of npm root -g
  3. require(`${yarn_global_dir}/node_modules/prettier`), where yarn_global_dir is the output of yarn global dir
aspiers commented 2 years ago

@jscheid commented on September 27, 2021 7:07 AM:

@aspiers the prettier-info dump suggests that the Prettier installation being picked up does not have prettier-plugin-solidity installed.

This was definitely true originally, because I figured that out myself, and then did a global install of the solidity plugin to fix it. And you can see from this result which I previously posted above that the plugin install already had the desired effect:

$ prettier --file-info /path/to/file.sol | jq .inferredParser
"solidity-parse"

The prettier-info dump currently doesn't include the location from where Prettier is picked up, which I'm going to improve.

Cool, thanks!

And I do grant you that it would probably be useful to surface in a more accessible way.

That being said, the logic is to find the closest package.json starting with the path of the file being formatted, and using the "local" Prettier installation from there if possible, falling back to the global prettier installation which is determined by trying, in this order:

  1. require("prettier")
  2. require(`${npm_root}/prettier`), where npm_root is the output of npm root -g
  3. require(`${yarn_global_dir}/node_modules/prettier`), where yarn_global_dir is the output of yarn global dir

OK, so maybe the discrepancy lies somewhere in this logic. Which is exactly the kind of thing I was referring to above when I wrote:

A third might be some discrepancy between running prettier as a one-off from the CLI vs. running it as a long-running server process.

So yes, surfacing this info somehow (in prettier-info or elsewhere) would be awesome :-) I'll happily be the guinea pig for any such new debug, because I can't figure out why it wouldn't be picking up the solidity plugin.

Also, even when it can't, is it really correct that prettier-last-parser ends up being set to "none", and no prettification happens despite the minor mode being enabled? Wouldn't it be more user-friendly if the user gets warned that no parser is active? Otherwise they are very likely to assume that it's working and that all their code is pretty-formatted without realising they are potentially introducing a mess of inconsistencies into the codebase.

jscheid commented 2 years ago

This was definitely true originally, because I figured that out myself, and then did a global install of the solidity plugin to fix it.

Oh, do you just need to M-x prettier-restart for it to pick up the new plugin?

aspiers commented 2 years ago

Wish it was that simple! Already tried multiple restarts...

jscheid commented 2 years ago

@aspiers I haven't forgotten about this, just been busy with other stuff but will try and get to this soon.

aspiers commented 2 years ago

Thanks a lot! I'm having quite a few issues with the mode at the moment and I can't figure out why, so some help would be super appreciated whenever you have time!

jscheid commented 2 years ago

@aspiers I've started working on this in #97: the first step is to add more info to prettier-info output. (We can still think about more human-friendly output later on.)

Alas, the build fails with the same error that you ran into. I've investigated and it so happens that #66 fixes the problem. Since I've been meaning to do something with #66 anyway, I've rebased it and I'm going to dogfood it over the next few days. If there aren't any issues I'm planning to merge it and then pick up #97 again after.

In the meantime you could help by also testing #66 (zipped tarball). Of course you can also use the WIP in #97 and see if it helps shed some light on the issue with solidity, but you'll have to compile/install it yourself.

PS I too couldn't reproduce the error locally at first. Switching to a Docker setup loosely based on this let me reproduce it so that's another improvement I'm planning to make.

aspiers commented 2 years ago

Sounds fantastic, I will be glad to help testing!

aspiers commented 2 years ago

I have merged these two PRs into a temporary branch for testing, and I guess the batching is working fine (at least I didn't notice any problems) but I'm unsure how to benefit from the increased info, as I see it has only been added to the Javascript not to the elisp?

I'm pretty desperate to figure out the configuration of the prettier process in question, because not only is it not correctly triggering for some Solidity files, it's also picking up the wrong config for some Javascript files, resulting in an endless war between emacs and the separate prettier process invoked by the husky precommit hook.

jscheid commented 2 years ago

That sounds painful. I'm sure we'll get to the bottom of it.

The increased info should appear in M-x prettier-info output which is more or less just a serialization of an object that the JS code generates, hence no changes on the Elisp side so far.

jscheid commented 2 years ago

Make sure you run make so the JS changes get picked up.

aspiers commented 2 years ago

OK I finally got it working with a custom recipe!

(use-package prettier
  :straight (prettier :type git :flavor melpa
                      :files (:defaults "dist/*")
                      :branch "master" :host github
                      :repo "jscheid/prettier.el"))

and now I see extra stuff in prettier-info. One issue I've immediately noticed is that if I first visit a file inside a yarn workspace, it starts with :context set to that workspace, rather than to the root of the repo. And if the prettier config is at the top level rather than in the workspace, this means that prettier.el won't use the right config. Any ideas how to handle that? I guess I should submit a separate issue for that.

jscheid commented 2 years ago

@aspiers yes would you mind opening a new issue? Could you mention which version of Yarn you're using and whether you're using PnP (I'm guessing not.)

jscheid commented 2 years ago

@aspiers still happy to take a look, but would be good if I could first recreate the issue here... for that I'd need to know your setup.