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

Make getSupportedCodeFixes plugin test such that we dont need to update its baseline for every new error #53070

Merged
merged 3 commits into from
Mar 2, 2023

Conversation

sheetalkamat
Copy link
Member

Take 2
#52596 still left one request that was not redirected so printing all the error codes.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 2, 2023
/** @internal */
export function getSupportedErrorCodes(): readonly string[] {
return arrayFrom(errorCodeToFixes.keys());
return errorCodeToFixesArray ??= arrayFrom(errorCodeToFixes.keys());
Copy link
Member

@jakebailey jakebailey Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codefixes are registered at runtime, so if this call happens too early it may get locked in forever with a reduced list.

Can we add something like errorCodeToFixesArray = undefined to registerCodeFix above just to make sure this function is always accurate? It's unlikely, but, seems like it'd be a bit annoying to have to debug if we did accidentally cause this to happen. 😅

(I want to get rid of this registration mechanism at some point, as it's side-effect based and could be redone post-modules, but, that's another change.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now how this is setup is that this is immutable list and is correct when called. I will go ahead and add assert in register code fix though

Copy link
Member

@jakebailey jakebailey Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow .. thats just misusing it.. their plugin instead needs to implement getSupportedCodeFixes on LS

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An assert would stop those cases from functioning, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An assert would stop those cases from functioning, right?

Yeah for now i have just gone ahead with resetting the list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear I'm not against breaking this particular mechanism of adding code fixes. I'm curious why they did this; hopefully it was just because they didn't know about getSupportedCodeFixes, but maybe there's a restriction there I'm not thinking of. tsec is one that I think is actively maintained. The roblox one is... confusing because it doesn't add anything.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The restriction is that IDEs don't use the feature so the function is never called and the proxy is useless. I created an issue: microsoft/vscode#219273

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't making more people depend on this an even worse direction?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which direction?
I don't have much opinion on the design, but I think the language server should have an official API to extend code actions.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to have these baselines fixed!

@sheetalkamat sheetalkamat merged commit b19cfc0 into main Mar 2, 2023
@sheetalkamat sheetalkamat deleted the stablegetSupportedCodeFixes branch March 2, 2023 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants