microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
99.42k stars 12.32k forks source link

[API] Breaking behavior at suggestions order from `getCompletionsAtPosition` (sorting suggestions) #49012

Open zardoy opened 2 years ago

zardoy commented 2 years ago

Bug Report

Update: I'm not intersted in this issue anymore as I make it available in TS plugin with this implementation.

I'm raising this issue not only because it wasn't mentioned in breaking changes for v4.6, but also because it broke editing experience, after my IDE updated builtin TS version (e.g. WebStorm)

🔎 Search Terms

breaking behavior suggestions sorting incorrent sorting webstorm incorrent suggestions sorting

🕗 Version & Regression Information

💻 Code

Sample Code (also used for bisect) ```ts //@ts-check const ts = require("./built/local/tsserverlibrary") const assert = require("assert") const testString = "const test: {b: 1, a: 2} = {}" const dummyProjectVersion = 1 const languageService = ts.createLanguageService({ getProjectVersion: () => dummyProjectVersion.toString(), getScriptVersion: () => dummyProjectVersion.toString(), getCompilationSettings: () => ({ allowJs: true, jsx: ts.JsxEmit.Preserve }), getScriptFileNames: () => ["/test.ts"], getScriptSnapshot: fileName => { return ts.ScriptSnapshot.fromString(testString) }, getCurrentDirectory: () => "", getDefaultLibFileName: () => "defaultLib:lib.d.ts", }) assert.deepStrictEqual(languageService.getCompletionsAtPosition("/test.ts", testString.length - 1, {}).entries.map(({ name }) => name), ["b", "a"]) ```

🙁 Actual behavior

Assert fails (a, b response).

🙂 Expected behavior

Assert doesn't fail (b, a response).

The general problem

IMO the general idea of forcefully sorting all suggestions is bad, as TS just can't predict what user wants "mostly" in some cases. The user itself or library maintainers should have the ability to control what will user see. Some real world examples:

import { defineStore } from 'pinia'
import { createSlice } from '@reduxjs/toolkit'

createSlice({
    |
})

MergedImages

Left: 4.6.3. Right: builtin 4.5.5

Futhermore, I used that response order to "fill up" missing fields. This was crazy, it was like giving a +200% speed boost. Starting from now, fields in new order is much harder to read (e.g. I need to search through the fields to just find the name of the store slice).

defineStore from pinia also have defined state first, because this is the first prop that makes sense to use. But as you can see from above, now TS service outputs it last.

Even better example would be gaining popularity defineConfig utils. Imagine some tool like Jest has a way to configure it in the following way:

//@filename jest.config.mjs
// not real code!
import { defineConfig } from 'jest'

export default defineConfig({
  |
})

Here, library maintainer can specify config fields in order, so most used/important config keys will appear earlier. Everybody would be happy: mainainer has things organized and for users it's easier to remember the setting name.

Pinging @andrewbranch as you posted that PR. As you said here https://github.com/microsoft/TypeScript/pull/46703/files#diff-4237fd08a5f920543e184b7748bc8ae000d9b1c031810ba375b0053362ec2b6aR302 , that "editor typically do alphabetically sort" - I don't think its true, AFAIK only VSCode does this. Futhermore, I was disabling this algorithm via TS plugin using proxy and enjoying correct sorting of suggestions everywhere:

prior.entries = prior.entries.map((entry, index) => ({ ...entry, sortText: `${entry.sortText ?? ''}${index}` }))

The most frustrating part that as I understand there absolutely no way to "turn on" old behavior. I'm just loosing information about original sorting of the fields (please correct if I'm wrong).

For now, I've ended up with patching TS source code -> diffing the output -> patching builtin VSCode tsserver to just restore this behavior, so I do hope you'll participate in this issue. I just want original sorting of suggestions, that's it!

andrewbranch commented 2 years ago

The original sorting of completions was arbitrary and sensitive to minor implementation changes, which made it too easy for us to break integration tests for VS. All your examples display in alphabetical order for me in TS 4.5.5. I’m open to the suggestion of returning object literal members in declaration order when possible as that seems to be the sorting you care most about. But we’ll have to find a way to cement that ordering into the sort text. We’re not going back to the chaos where the order returned by the language service is different from the order returned by TS Server which is different from the order VS Code displays.

zardoy commented 2 years ago

All your examples display in alphabetical order for me in TS 4.5.5.

Hey, as I said above, I've been using TS Plugin in VSCode to achieve that. However, I just published a repo for quick testing: https://github.com/zardoy/ts-sorting-plugin. Just open it in VSCode and after installing deps you should see the original order of suggestions in index.ts (as VSCode's ext would load the plugin). Don't forget to switch to project TS. By the way when I posted this, I missed the sides on first screenshot, sorry for that, now fixed.

I’m open to the suggestion of returning object literal members ...

But what about property dot access? There were a lot of cases where original order of completions was useful.

We’re not going back to the chaos where the order returned by the language service is different from the order returned by TS Server which is different from the order VS Code displays.

I'm totally fine with that, but at very least it would be great to have an option, that I can pass to getCompletionsAtPosition like disableSorting.

Also, there is another interesting thing. There is builtin code action called Add missing properties that would place missing properties in the right order.

andrewbranch commented 2 years ago

But what about property dot access?

Sure.