Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change default of onSave to true? #55

Closed
karlhorky opened this issue Aug 18, 2020 · 9 comments
Closed

Change default of onSave to true? #55

karlhorky opened this issue Aug 18, 2020 · 9 comments

Comments

@karlhorky
Copy link

karlhorky commented Aug 18, 2020

Hi there, first of all, thanks so much for this extension. Saves so much time and is great for teaching!

I was wondering if you would consider a change of the default of the onSave option to true instead of false. It would make the "out of the box" UX much nicer, since the lens won't complain about errors as you are still typing.

Just trying this option out, I see that it doesn't extend to files that have just been opened. Maybe this could also be part of the default? I'm thinking of a default behavior something like:

  1. Check for errors and show lenses when the user opens a file (opened separately in Check for errors and show lenses when the user opens a file #57)
  2. Check for errors and show lenses when the user saves a file

What are your thoughts?

@karlhorky
Copy link
Author

karlhorky commented Aug 18, 2020

Oh, I just realized, this setting is a bit buggy sometimes. Sometimes TypeScript diagnostics come back slowly after save, and the extension does not update them.

For example, here the diagnostics from TypeScript are already gone and resolved (no squiggly underlines), but the lenses are still there 😟

Screen Shot 2020-08-18 at 11 09 02

I guess this would need to be fixed if it would become the default...


If I should open another issue for this, happy to do so.

@usernamehw
Copy link
Owner

usernamehw commented Aug 18, 2020

I was wondering if you would consider a change of the default of the onSave option to true instead of false.

Changing the default won't happen because I believe a lot of people are using vscode setting "files.autoSave": "afterDelay" that makes this feature behave unpredictable (decorations would render by interval timer).

Check for errors and show lenses when the user opens a file

This can be easily added.

Sometimes TypeScript diagnostics come back slowly after save

Do you have an approximate time of how slow they are? I can increase interval at

if (Date.now() - Global.lastSavedTimestamp < 1000) {

from 1000 to, perhaps, 3000. But this will make all the diagnostics change events to render for 3 seconds after the save. Can possibly add a setting for this delay.

@karlhorky
Copy link
Author

Changing the default won't happen because I believe a lot of people are using vscode setting "files.autoSave": "afterDelay" that makes this feature behave unpredictable

Ah, that's understandable. How about setting default to: onSave: files.autoSave !== 'afterDelay'?

Do you have an approximate time of how slow they are?

Ah yeah this is very different, based on a lot of factors (size of the project, what else is running on the computer, etc).

I guess a callback-based approach would be better. I don't know too much about VS Code extension development, but maybe using a "save provider" or similar would work? I'm thinking, some way to do work after all other save providers have done things.

Some ideas:

  1. In the early versions of prettier-vscode, they used workspace.onWillSaveTextDocument (see https://github.com/prettier/prettier-vscode/blob/0.6.0/src/extension.ts#L23) ...
  2. ...although then they switched to DocumentFormattingEditProvider afterwards (https://github.com/prettier/prettier-vscode/blob/725e9bbd137cc5b0699d90f6ab12df91f36af05f/src/PrettierEditProvider.ts#L16)

@karlhorky
Copy link
Author

Ok I split out the one request into a separate issue (#57) here for easier tracking:

  1. Check for errors and show lenses when the user opens a file

@usernamehw
Copy link
Owner

  • Extension now uses onWillSave event and that makes it work even when auto save is enabled
  • There's a new setting onSaveTimeout for that interval after save

You can try it out and leave feedback if it works better than before:

errorlens-3.2.0.zip

Change file extension from .zip to .vsix to install.

The default value of onSave remains the same. I don't think it will ever be changed. Sorry.

@karlhorky
Copy link
Author

Trying this now, and it is working well, thanks! 🙌

Since the default will stay with false, I'll close this issue then.

I guess 3.3.0 will be the release with these changes?

@karlhorky
Copy link
Author

I've also asked the VS Code project whether they'd be open for adding a reason property on DiagnosticChangeEvent, which would allow removal of the onSaveTimeout check:

microsoft/vscode#105140

karlhorky added a commit to karlhorky/dotfiles that referenced this issue Aug 21, 2020
@usernamehw
Copy link
Owner

I don't think it works that way. The diagnostic changes when the document changes - it doesn't know/care about document saves.

@karlhorky
Copy link
Author

I suppose that is possible. It would be nice to be able to trace the diagnostic change event back to what event caused them though (kind of like a stack trace).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants