search

jscheid / prettier.el

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

Pre-warm idea #81

Open dabrahams opened 3 years ago

dabrahams commented 3 years ago

Just a thought: why not start pre-warming in the background on first activation and wait for it (if it hasn't completed already) on first save? That would seem to give the ideal user experience in most cases.

jscheid commented 3 years ago

Great idea, it seems so obvious that I suspect there might be a reason why I didn't do it that way, as I remember spending quite a bit of time on tweaking the experience around first use back when I started out. But it's also possible it simply didn't occur to me. Let me take a closer look at this sometime soon.

jscheid commented 3 years ago

What I've found out so far revisiting my code:

  1. It turns out that this is actually how I had intended for it to work, except that there's a bug which renders it useless: the warm-up message is sent in a generator function, but because iter-next isn't called on the resulting iterator, the function -- although it will be invoked -- is never actually executed and therefore the warm-up message never sent.
  2. Even if it were to work correctly, the subprocess is only started when a relevant buffer is visited (or is already open at the time global-prettier-mode is activated) -- but in this case, the subprocess is (in the default configuration) queried immediately for Prettier settings, which means that any background warm-up activity won't have time to complete before the next request comes in, and so will effectively be blocking anyway.

Aside from fixing that bug, perhaps an improvement would be a setting that starts the process at load time rather than at first use. Perhaps a new setting eager for prettier-pre-warm, and it would probably be OK to enable that by default, as presumably people will only load (as opposed to install) the package if/when they intend to use it. What do you think?

dabrahams commented 3 years ago

Sounds like a good compromise.

P.S. Which code contains the bug that renders the strategy useless? Have you reported it?

On Thu, Mar 4, 2021 at 3:27 AM Julian Scheid notifications@github.com wrote:

What I've found out so far revisiting my code:

  1. It turns out that this is actually how I had intended for it to work, except that there's a bug which renders it useless: the warm-up message is sent in a generator function, but because iter-next isn't called on the resulting iterator, the function -- although it will be invoked -- is never actually executed and therefore the warm-up message never sent.
  2. Even if it were to work correctly, the subprocess is only started when a relevant buffer is visited (or is already open at the time global-prettier-mode is activated) -- but in this case, the subprocess is (in the default configuration) queried immediately for Prettier settings, which means that any background warm-up activity won't have time to complete before the next request comes in, and so will effectively be blocking anyway.

Aside from fixing that bug, perhaps an improvement would be a setting that starts the process at load time rather than at first use. Perhaps a new setting eager for prettier-pre-warm, and it would probably be OK to enable that by default, as presumably people will only load (as opposed to install) the package if/when they intend to use it. What do you think?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jscheid/prettier.el/issues/81#issuecomment-790544797, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKYIJG35A6LXIW5QEHOWLTB5VATANCNFSM4YRQGXYA .

-- -Dave

jscheid commented 3 years ago

It's a bug in this package. The iterator shouldn't be discarded here: https://github.com/jscheid/prettier.el/blob/ae5553b720c03e200440ec338318ecad52fe9640/prettier.el#L891

I'll fix that as part of implementing eager pre-warming.

angrybacon commented 2 years ago

Posting here as this is seems somewhat related: https://prettier.io/blog/2022/06/14/2.7.0.html

This release includes a new --cache CLI option. Enabling this option will use some attributes as cache keys and format files only if they have changed. This could dramatically improve CLI performance.

Can this be leveraged? I'll admit that I only briefly looked at the source out of curiosity but don't have a MR to offer.

(not that I have any performance issues at the moment)

jscheid commented 2 years ago

Can this be leveraged?

Ah, thanks for the heads up, however this package isn't using the Prettier CLI, it's using the API.

At any rate, the intended usage is to format on save, and Emacs doesn't save unchanged files (it will print (No changes need to be saved) if you try.) Therefore, something like this would be beneficial only if you run prettier-prettify manually without changing anything, or if you make a no-op change (e.g. insert a space and then delete it.) Those seem like corner-cases to me that aren't really worth optimizing for, would you agree?

angrybacon commented 2 years ago

Definitely, thanks for the clarification 👍

dabrahams commented 2 years ago

I'll fix that as part of implementing eager pre-warming.

Did you ever implement the eager pre-warming? Should we close this issue?

jscheid commented 2 years ago

@dabrahams I've actually started working on it recently as part of a major overhaul which I think of as v2.0. I've been working on it on and off and it's coming along nicely, watch this space.

It's going to be a variation of what we had discussed before -- specifically, in the (default) case where we sync over settings from Prettier, such as indentation width, I think we should block on first edit by default rather than on save.

This should still give a smooth experience in the common case where you don't immediately start editing a buffer after it opens. You'll usually navigate inside the buffer first or do something else entirely, such as opening a second file.