checkOnChange should be debounced, not throttled

[Arti-Art]

I would like to address the way the plugin checks the syntax, especially with the checkOnChange setting. When a character is typed in any liquid file, it will run a theme check on absolutely EVERYTHING in the working directory, and this check will not run again until the current check has finished. Here are the problems:

• When a character is typed and all subsequent characters are typed before all directories are finished being scanned, there will be no re-scan for those subsequent characters. Ex: If I type "crocodile", the scan starts with the letter "C", and if I finish typing "ROCODILE" before the scan completes, we have done a complete check of all directories only for the letter "C", not the whole word "crocodile. This looks highly inefficient.
• Looks like this issue is the reason that I often get warnings end errors when there should be none, and they disappear only after I save the file (because my checkOnSave is activated). The theme check often runs on incomplete syntax, and often skips the check when what I was trying ty type is complete.
• When a character is typed, ALL of the directories get scanned. Is it necessary to scan everything? My computer is very fast, but 'm sure this process is causing significant performance issues for others. If I am working in a single file for the whole day, I still end up having scanned all of my folders and files several hundreds times by the end of the the day!

So, to recap, theme check runs when it doesn't need to. Theme check does NOT run when it NEEDS to. Theme check constantly scans files that have not changed.

Example video: https://www.youtube.com/watch?v=2HRjf2plJws

[charlespwd]

It definitely is. And there's a lot of work that could be done to improve it. If you absolutely hate it, you can turn this feature off.

The reason it exists and is there is only so that you can apply quick-fixes from the editor. Since the quickfix needs to know where in the file the diagnostic is for.

Say you modified a JSON object and didn't save, you wouldn't be able to quickfix for format until you click save. You can turn it off and deal with it. Only being able to quickfix after a file save is an OK workflow. It's more efficient if you know about it. But it's also kind of hard to document and not everyone reads the documentation.

So... we run the diagnostics on every change. But there's an asterisk—and this is probably a problem because we look like we're running all the diagnostics when in fact we're not—we're going through all the files only for the checks that require an entire theme to run (e.g. unused translations, etc.). It's faster than a dumb full theme check, but the progress bar gives you the impression we're running a full theme-check. Should probably revisit that. You're not the first one to report this.

The first issue you talk about is because it is throttled and not debounced. That I should actually fix.

[vfonic]

I just came here to report this same issue with full theme check. Good to know it's not full theme check, but you can still see that it's quite slow. It takes 1+ second on my computer, which I consider very slow for "check on change" aka "instant feedback".

The scanning being throttled and not debounced is also an issue I've noticed.

[charlespwd]

I think I have something that will work for everyone:

How about we have another config—"themeCheck.onlySingleFileChecks"—that, when true, disables whole theme checking entirely. That means no MissingTranslations, UnusedSnippets, etc. It also means that the problem tab will only ever show the errors for the files that are opened.

This should speed up execution drastically (a theme with 1k+ errors runs single file check in 0.01ms, the whole theme check takes ~7s) at the small cost of not having all the errors all of the time. We could also, maybe, only do the single file checks on change, and somehow debounce the full theme check when we're done. Or only do full theme checks on save.

[vfonic]

This should speed up execution drastically (a theme with 1k+ errors runs single file check in 0.01ms

😍 This would be great! :)

This config could live along the other config settings. It's like "focus" mode for your specs. :) The developers (aka users) can then decide when they'd want to run the full check, maybe as a pre-commit hook and/or on CI, maybe on file save, etc.

I wouldn't make it so that "themeCheck.onlySingleFileChecks" implies that full theme check would be run on save.

Maybe existing config options can be changed from boolean to string:

  "themeCheck.checkOnChange": "onlyCurrentFile | openFiles | allFiles",
  "themeCheck.checkOnOpen": "onlyCurrentFile | openFiles | allFiles",
  "themeCheck.checkOnSave": "onlyCurrentFile | openFiles | allFiles",

I don't know if all of the options would be useful for all of these settings and how would the migration from boolean to string work for a VS Code extension config.

[charlespwd]

I wouldn't make it so that "themeCheck.onlySingleFileChecks" implies that full theme check would be run on save.

I meant this: if the option is off, then we're going to do the fast thing while you type, and the slow thing on save. If onlySingleFileChecks is on, it's always on. (Maybe the cmd+shift+p Run checks command could force run all of them though).

[charlespwd]

Maybe existing config options can be changed from boolean to string:

Not a bad idea. But it might make things weird when you have leftovers from a full theme check mixing with single file checks. Do we keep the leftovers? Do we clear them? Is this really a problem we can't solve?

However, I don't think the LSP provides us with a mechanism to distinguish between open and current file unfortunately :/ But I'll try and confirm if there isn't some kind of meta data in the textDocument/didOpen notification.

[charlespwd]

Yeah. It comes in as this:

[Trace - 11:48:49 AM] Sending notification 'textDocument/didOpen'.
Params: {
    "textDocument": {
        "uri": "file:///Users/charles/ws/theme/sections/text-and-image.liquid",
        "languageId": "liquid",
        "version": 1,
        "text": "...filecontents..."
    }
}

[vfonic]

I meant this: if the option is off, then we're going to do the fast thing while you type, and the slow thing on save. If onlySingleFileChecks is on, it's always on. (Maybe the cmd+shift+p Run checks command could force run all of them though).

I agree, only currently open file should be checked on change.

I was thinking more about how other code analyzing tools work. Especially whether they analyze more than one file and/or basically anything outside of a single file.

The more I think of it, the more I believe the answer is: No.

If I use Prettier, it will only format the current file on save. If I use ESLint, it will only lint the currently open file. Same if I use Stylelint.

I believe this is an experience that developers are used to. Developers expect this tool to behave similarly. Can I run ESLint on the whole project? Sure. Can I run theme-check on the whole project? Yes, but when I want to do so: via CLI command and/or on CI.

[charlespwd]

I think I agree but, at the same time, why prevent it? We could have both with "themeCheck.onlySingleFileChecks".

• When true, it behaves exactly as you expect it. Only ever report single file checks for the open files.
• When false, it behaves as it did. And now that I figured out a way to have fast checks while typing, it won't feel slow!

true is probably a better default but I'm not sure pushing a workflow-breaking change that requires a config update to get what you already had is the way to go. And for that reason, I feel like the switch should be intentional 😬 (?)

Check this out:

• `"themeCheck.onlySingleFileChecks": false

  https://user-images.githubusercontent.com/4990691/155549947-3274d4b0-ee8a-4fc5-a76c-584a014055d3.mp4

• `"themeCheck.onlySingleFileChecks": true

  https://user-images.githubusercontent.com/4990691/155550962-a1b0dd41-de3e-4912-b836-b0c7d35655d5.mp4

[vfonic]

This is f*in' awesome! 😍😍😍 :))))

Maybe you could call that config setting more affirmative. Instead of onlySingleFileChecks, you could call it performFullThemeCheck or similar? 🤔

Either way, I'm downloading this change right now! :)

[vfonic]

It works great! Thank you!

I didn't even have to install dev version of VS Code extension. I just downloaded the changes in this repo (theme-check), built the gem and installed it locally. Then I added the "unknown"/"non-existing" setting option in VS Code settings JSON:

"themeCheck.onlySingleFileChecks": true

And it worked! :)

[charlespwd]

Give me a chance to make a new release hahaha :D Glad you like it! This kind of reaction makes my day :)

[vfonic]

Sorry about that! :D I was too excited to give it a try.

I really love this change! 🚀

Now I can leave theme-check server running in the background, constantly (and immediately) checking the code.

What's next? Are you planning on adding some formatting? Perhaps something that could be set to run on save? Like these things:

{%-for product in products-%}

=>

{%- for product in products -%}

[charlespwd]

You should already be able to fix those with the quickfix/autofix keybind :)

Quickfix for the popup. Autofix for the "preferred" fix (usually the one below the cursor).

https://user-images.githubusercontent.com/4990691/155567649-991fb185-f656-4072-8992-85022ae1ecaa.mp4

[charlespwd]

As for a formatter, yep it's on my TODO (got a secret prettier plugin prototype somewhere) but it's really hard to get right 🙃 and I don't have enough bandwidth to maintain more stuff. We're hiring though so hopefully that will unlock more stuff in the near future :)

[vfonic]

Awesome! I've just set up a keyboard shortcut for "Fix all" command.

Thanks again! 🙏