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

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

Open
zardoy opened this issue May 7, 2022 · 3 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Domain: Completion Lists The issue relates to showing completion lists in an editor Suggestion An idea for TypeScript

Comments

@zardoy
Copy link
Contributor

zardoy commented May 7, 2022

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-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
Copy link
Member

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.

@andrewbranch andrewbranch added Suggestion An idea for TypeScript Domain: Completion Lists The issue relates to showing completion lists in an editor Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels May 9, 2022
@zardoy
Copy link
Contributor Author

zardoy commented May 10, 2022

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
Copy link
Member

But what about property dot access?

Sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Domain: Completion Lists The issue relates to showing completion lists in an editor Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

2 participants