Skip to content
This repository has been archived by the owner on Nov 5, 2021. It is now read-only.

Adds a CodeAction provider to support fixits #40

Merged
merged 6 commits into from
Oct 2, 2019

Conversation

orta
Copy link
Contributor

@orta orta commented Aug 29, 2019

gifs 2019-08-29 13_35_46

Adds the ability to showcase code actions and suggestions in Monaco-typescript - woo! Now it's feasible for the TS team to showcase code edits :D

if (!noSyntaxValidation) {
promises.push(worker.getSyntacticDiagnostics(resource.toString()));
}
if (!noSemanticValidation) {
promises.push(worker.getSemanticDiagnostics(resource.toString()));
}
if (!noSuggestionDiagnostics) {
promises.push(worker.getSuggestionDiagnostics(resource.toString()));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also means that JS files should get suggestions too

@@ -188,14 +191,24 @@ export class DiagnostcsAdapter extends Adapter {
const { lineNumber: endLineNumber, column: endColumn } = this._offsetToPosition(resource, diag.start + diag.length);

return {
severity: monaco.MarkerSeverity.Error,
severity: this._tsDiagnosticCategoryToMarkerSeverity(diag.category),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously we'd only show only errors, now it can be of many types

const start = this._positionToOffset(resource, { lineNumber: range.startLineNumber, column: range.startColumn });
const end = this._positionToOffset(resource, { lineNumber: range.endLineNumber, column: range.endColumn });

// TODO: where to get the current formatting options from?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one I'm still not sure of, it doesn't feel critical - but if anyone has ideas on what the right way to pass in the formatting options, I'd love to know them

kind: codeFix.fixName
};

return action;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is my best-guess implementation, leaving most of the work to the edits which come with the original TS code action ideas

@@ -200,6 +206,11 @@ export class TypeScriptWorker implements ts.LanguageServiceHost {
return Promise.resolve(this._languageService.getEmitOutput(fileName));
}

getCodeFixesAtPosition(fileName: string, start: number, end: number, errorCodes:number[], formatOptions: ts.FormatCodeOptions): Promise<ReadonlyArray<ts.CodeFixAction>> {
const preferences = {}
return Promise.resolve(this._languageService.getCodeFixesAtPosition(fileName, start, end, errorCodes, formatOptions, preferences));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think its worth exposing all of the TS code fix options to devs, but I'm open to changing that?

it looks like this:

interface UserPreferences {
        readonly disableSuggestions?: boolean;
        readonly quotePreference?: "auto" | "double" | "single";
        readonly includeCompletionsForModuleExports?: boolean;
        readonly includeCompletionsWithInsertText?: boolean;
        readonly importModuleSpecifierPreference?: "relative" | "non-relative";
        /** Determines whether we import `foo/index.ts` as "foo", "foo/index", or "foo/index.js" */
        readonly importModuleSpecifierEnding?: "minimal" | "index" | "js";
        readonly allowTextChangesInNewFiles?: boolean;
        readonly providePrefixAndSuffixTextForRename?: boolean;
}

test/index.html Outdated
@@ -165,10 +165,11 @@ <h2>Monaco Editor TypeScript test page</h2>
'var game = new Conway.GameOfLife();',

].join('\n'),
language: 'typescript'
language: 'typescript',
lightbulb: { enabled: true }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took 30m guessing, then 5m with a debugger on the plane to figure this one out! 🍡 💡

@alexdima alexdima self-requested a review September 19, 2019 07:42
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

Thank you, this looks great. Two quick problems:

  1. The hover always shows that no quick fixes are available.
  2. I get an error when applying a quick fix

@mjbvz Can you please help out with these two things, I don't know what would cause them...

image

image

@orta
Copy link
Contributor Author

orta commented Sep 26, 2019

I've looked at this a second time but not entirely got the answer. I think what's happening here is that because I send both the edits and a command to run, then the edits are applied but the command after fails which shows that error.

From a look in vscode, it looks like instead it uses a custom command that sends messages which are more or less just sent straight back to the typescript worker to do the editing changes instead.

Will take a stab at that

@orta
Copy link
Contributor Author

orta commented Sep 26, 2019

This... Is tricky.

The ts/js language implementations have no access to an instance of an editor. To handle the callback in a similar manner to vscode, I need an instance of the editor to register a command for the code actions to run.

There are two routes I can do as-is:

  1. Add an extra call to monaco.lanaguges.typescript which passes in an editor. e.g.

    const editor = monaco.editor.create({...})
    monaco.editor.languages.typescript.setup(editor)

    Which could be used to add the commands and to opt the editor into getting code action support.

  2. Use monaco.editor.onDidCreateEditor to add the TypeScript command, this means adding it to every instance of the editor regardless of whether you're using TS or not (because a model can change language to typescript at runtime ( I think ) )

Neither are great options, though I may go for the former.

@spahnke
Copy link
Contributor

spahnke commented Sep 30, 2019

Do you need to register a command here at all? From the documentation in vscode.d.ts I get the impression that edits would be sufficient. At least that's the only thing I supply when hooking up ESLint to Monaco. It would be interesting to know what an additional command would be used for.

@spahnke
Copy link
Contributor

spahnke commented Sep 30, 2019

Also: The kind property has to be set to "quickfix". Otherwise the popup says "No quick fixes available". This is not documented unfortunately and it took me a while to find that (I think I debugged, or looked into the source code).

@orta
Copy link
Contributor Author

orta commented Sep 30, 2019

Do you need to register a command here at all?

I do if I want to let typescript handle making the changes to the code (which is what happens in vscode ) - there's no other route for getting the callback that a user has selected it.

Maybe I can just have a version which just applies the code edits which TypeScript passes and call it at that.

Also: The kind property has to be set to "quickfix".

<3

@orta
Copy link
Contributor Author

orta commented Oct 1, 2019

Screen Recording 2019-10-01 09_11_37

Thanks @spahnke - I've tried this out on a bunch of our fixits and it works as expected.

This version doesn't use the TypeScript services to make the changes, but only uses the code edit deltas sent from the original response from the language service which seems to be enough for every fixits I tried from in the typescript test suite.

Should be good to go.

@alexdima
Copy link
Member

alexdima commented Oct 2, 2019

This looks great! ❤️

@alexdima alexdima merged commit c050127 into microsoft:master Oct 2, 2019
@alexdima alexdima added this to the September 2019 milestone Oct 2, 2019
@orta
Copy link
Contributor Author

orta commented Oct 2, 2019

Woo, thanks @alexandrudima <3

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants