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

feat: Add insert replace support for completions #583

Merged
merged 13 commits into from
Sep 6, 2022
Merged

feat: Add insert replace support for completions #583

merged 13 commits into from
Sep 6, 2022

Conversation

predragnikolic
Copy link
Contributor

This PR adds insert replace support.

src/completion.ts Outdated Show resolved Hide resolved
@predragnikolic
Copy link
Contributor Author

I had some issues with committing locally,

husky > pre-commit (node v16.16.0)
$ eslint --ext ".js,.ts" src
$ concurrently -n compile,lint -c blue,green "yarn compile" "yarn lint"
[lint] $ eslint --ext ".js,.ts" src
[compile] $ tsc -b
[compile] yarn compile exited with code 0
[lint] yarn lint exited with code 0
$ cross-env CONSOLE_LOG_LEVEL=warn mocha "./lib/**/*.spec.js"
(node:97369) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
(node:97369) ExperimentalWarning: The Node.js specifier resolution flag is experimental. It could change or be removed at any time.

ReferenceError: exports is not defined in ES module scope
This file is being treated as an ES module because it has a '.js' file extension and '/home/predragnikolic/Documents/sandbox/typescript-language-server/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.
    at file:///home/predragnikolic/Documents/sandbox/typescript-language-server/lib/modules-resolver.spec.js:27:23
    at ModuleJob.run (node:internal/modules/esm/module_job:198:25)
    at async Promise.all (index 0)
    at ESMLoader.import (node:internal/modules/esm/loader:385:24)
    at importModuleDynamicallyWrapper (node:internal/vm/module:437:15)
    at formattedImport (/home/predragnikolic/Documents/sandbox/typescript-language-server/node_modules/mocha/lib/nodejs/esm-utils.js:7:14)
    at Object.exports.requireOrImport (/home/predragnikolic/Documents/sandbox/typescript-language-server/node_modules/mocha/lib/nodejs/esm-utils.js:38:28)
    at Object.exports.loadFilesAsync (/home/predragnikolic/Documents/sandbox/typescript-language-server/node_modules/mocha/lib/nodejs/esm-utils.js:91:20)
    at singleRun (/home/predragnikolic/Documents/sandbox/typescript-language-server/node_modules/mocha/lib/cli/run-helpers.js:125:3)
    at Object.exports.handler (/home/predragnikolic/Documents/sandbox/typescript-language-server/node_modules/mocha/lib/cli/run.js:370:5)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
husky > pre-commit hook failed (add --no-verify to bypass)

Probably because I am using Node v14.18.3, and in the logs I can see node v16.16.0
so I just committed with --no-verify.

Is that Ok?
Or should I take some actions and if so what exactly?

@rchl
Copy link
Member

rchl commented Sep 5, 2022

Will repeat myself for the record:

I'm a bit concerned about doing that as it will add a ton of extra bytes to the response which can already be in megabytes...

On the other hand, if it's just the correct thing to do, we might have to...

@rchl
Copy link
Member

rchl commented Sep 5, 2022

so I just committed with --no-verify.

Is that Ok?

It's OK but you still have to fix the CI errors so you might as well just switch to Node 16. :)
Repo has .nvmrc so it will switch automatically if you are using nvm.

src/ts-protocol.ts Outdated Show resolved Hide resolved
src/lsp-server.ts Outdated Show resolved Hide resolved
src/lsp-server.ts Outdated Show resolved Hide resolved
src/lsp-server.ts Outdated Show resolved Hide resolved
src/completion.ts Outdated Show resolved Hide resolved
src/completion.ts Outdated Show resolved Hide resolved
@predragnikolic
Copy link
Contributor Author

I'm a bit concerned about doing that as it will add a ton of extra bytes to the response which can already be in megabytes...

LSP spec to the rescue :)
They added something new to the CompletionList type.

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionList

/**
	 * In many cases the items of an actual completion result share the same
	 * value for properties like `commitCharacters` or the range of a text
	 * edit. A completion list can therefore define item defaults which will
	 * be used if a completion item itself doesn't specify the value.
	 *
	 * If a completion list specifies a default value and a completion item
	 * also specifies a corresponding value the one from the item is used.
	 *
	 * Servers are only allowed to return default values if the client
	 * signals support for this via the `completionList.itemDefaults`
	 * capability.
	 *
	 * @since 3.17.0
	 */
	itemDefaults?: {
		/**
		 * A default commit character set.
		 *
		 * @since 3.17.0
		 */
		commitCharacters?: string[];

		/**
		 * A default edit range
		 *
		 * @since 3.17.0
		 */
		editRange?: [Range](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#range) | {
			insert: [Range](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#range);
			replace: [Range](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#range);
		};

		/**
		 * A default insert text format
		 *
		 * @since 3.17.0
		 */
		insertTextFormat?: [InsertTextFormat](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#insertTextFormat);

		/**
		 * A default insert text mode
		 *
		 * @since 3.17.0
		 */
		insertTextMode?: [InsertTextMode](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#insertTextMode);

		/**
		 * A default data value.
		 *
		 * @since 3.17.0
		 */
		data?: [LSPAny](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#lspAny);
	}

src/completion.ts Outdated Show resolved Hide resolved
Co-authored-by: Rafał Chłodnicki <rchl2k@gmail.com>
@predragnikolic
Copy link
Contributor Author

Re-guarding the new spec itemDefaults I do not think that I will implement it as part of this PR.
I'll continue with the PR.
The itemDefaults is a really nice addition to the spec, but I suppose there will be a future PR that will add support for that.

@predragnikolic predragnikolic marked this pull request as ready for review September 6, 2022 08:04
src/completion.ts Outdated Show resolved Hide resolved
@rchl rchl changed the title feat: Add insert replace support feat: Add insert replace support for completions Sep 6, 2022
@rchl rchl merged commit fdf9d11 into typescript-language-server:master Sep 6, 2022
@rchl
Copy link
Member

rchl commented Sep 6, 2022

Thanks!

@predragnikolic predragnikolic deleted the feat/insert-replace-support branch September 6, 2022 19:24
@rchl
Copy link
Member

rchl commented Sep 9, 2022

Been running with it for a while and I'm a bit annoyed by the new behavior (this is without corresponding LSP changes).

For a code like:

return path.reso|scriptArgs[0] || './.lokalise';

where | is the cursor it will replace the following text and end up with

return path.resolve[0] || './.lokalise';

I'm not sure if I'm not used to it or it's just annoying...

@predragnikolic
Copy link
Contributor Author

Maybe the default should be insert
instead of replace
https://github.com/typescript-language-server/typescript-language-server/pull/583/files#diff-cc3c09cd0e907b36fa79b43e743693740528eeaae34247576656613a9b115935R116

- item.textEdit = lsp.TextEdit.replace(replacementRange, insertText);
+ item.textEdit = lsp.TextEdit.replace(insertRange, insertText);

@rchl
Copy link
Member

rchl commented Sep 9, 2022

I don't know, that would be also a behavior change and potentially breaking. It's just that previously this happened only in specific cases while now it happens (almost?) every time.

Alternatively we could only respect/use optionalReplacementSpan when client supports InsertReplace.

@predragnikolic
Copy link
Contributor Author

Alternatively we could only respect/use optionalReplacementSpan when client supports InsertReplace.

Yes, that is the best way to keep the prev behavior.

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

Successfully merging this pull request may close these issues.

None yet

2 participants