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

[Take Over mode][Suggestion] Add Setting to Force Suggestions Ordering #1005

Closed
zardoy opened this issue Mar 2, 2022 · 19 comments
Closed

[Take Over mode][Suggestion] Add Setting to Force Suggestions Ordering #1005

zardoy opened this issue Mar 2, 2022 · 19 comments
Labels
wontfix This will not be worked on

Comments

@zardoy
Copy link
Contributor

zardoy commented Mar 2, 2022

Some Background

This is a continuation of #974 (comment) and comment below.

You just said

We need to be consistent with VSCode, because their designed behavior is verified by more users, and changing it in volar might be counterproductive.

However the behavior is already inconsistent, e.g. Volar is already much bettter comparing to what we have with builtin TS extension right now. I also noticed that Volar loads much faster and overall has better performance.

I have a repo, where builtin TS extension gives false errors (suppose just crashes) on pnpm start until I restart the server and this is not a case with your Volar.

So I always recommend to everyone to disable builtin TS extension globally in favor of takeover mode.

Feature Request

By default vscode agressively sorts everything by label, so initial order of suggestions is lost.

I'm just asking you to add a volar.extended.forceSortSuggestions setting. Here https://github.com/johnsoncodehk/volar/blob/master/packages/server/src/features/languageFeatures.ts#L45 add

if (list) list.items.forEach((item, i) => {
    item.sortText = `${item.sortText ?? ''}${i}`
})

Here is a code you can test with:

interface Foo {
	b: string,
}

interface Bar {
	c: boolean,
}

interface Test extends Foo, Bar {
	a: number
}

const d: Test = {
// trigger suggestions here
};

The setting will be disabled by default, so we don't break anything for existing users.

@johnsoncodehk
Copy link
Member

So I always recommend to everyone to disable builtin TS extension globally in favor of takeover mode.

I don't really suggest that, at least that's not the purpose of volar. 😅
Maybe the vscode TS extension has some overhead performance issues, I suggest you keep to check if it resolve in new vscode version.


Do you expected b -> c -> a in this example? I checked TS original completion response at https://github.com/johnsoncodehk/volar/blob/01adff94b3851be652a4f463323a5c340e8a0596/packages/vscode-typescript-languageservice/src/services/completion.ts#L49-L51, it seems already sort by label. So it can't change in vscode and volar in this case.

Do you have other example or reference issue for the problem?

[
  {
    name: 'a',
    kind: 'property',
    kindModifiers: '',
    sortText: '11',
    source: undefined,
    hasAction: undefined,
    isRecommended: undefined,
    insertText: undefined,
    replacementSpan: undefined,
    sourceDisplay: undefined,
    isSnippet: undefined,
    isPackageJsonImport: undefined,
    isImportStatementCompletion: undefined,
    data: undefined
  },
  {
    name: 'b',
    kind: 'property',
    kindModifiers: '',
    sortText: '11',
    source: undefined,
    hasAction: undefined,
    isRecommended: undefined,
    insertText: undefined,
    replacementSpan: undefined,
    sourceDisplay: undefined,
    isSnippet: undefined,
    isPackageJsonImport: undefined,
    isImportStatementCompletion: undefined,
    data: undefined
  },
  {
    name: 'c',
    kind: 'property',
    kindModifiers: '',
    sortText: '11',
    source: undefined,
    hasAction: undefined,
    isRecommended: undefined,
    insertText: undefined,
    replacementSpan: undefined,
    sourceDisplay: undefined,
    isSnippet: undefined,
    isPackageJsonImport: undefined,
    isImportStatementCompletion: undefined,
    data: undefined
  }
]

@zardoy
Copy link
Contributor Author

zardoy commented Mar 3, 2022

Do you expected b -> c -> a in this example? I checked TS original completion response at

Interesting...

But I definitely see the deffirence with pinia package:

Unmodified Volar:
Screenshot 2022-03-03 at 20 32 05

With the change from first comment:

Screenshot 2022-03-03 at 20 35 03

This is something that WebStorm does by default. However WebStorm shows correct order of suggestions only when some time passes after IDE load.

Another example would be createSlice from @reduxjs/toolkit (root export). First property that makes sense to define is a name of store slice. Both WebStorm and Volar with these changes show name as first suggestion.

I don't really know how to print the item list from https://github.com/johnsoncodehk/volar/blob/master/packages/server/src/features/languageFeatures.ts#L45. How did you check that? I tried placing breakpoint in several places, but they didn't work.

Anyway, can you install pinia and reproduce it from code above? Or I need to publish the full repository of repro?

@johnsoncodehk
Copy link
Member

@zardoy since changed yarn to pnpm breakpoint just not no longer working, you could rollback to d222365 to debug, I will fix it later. I just log it by console.log(completionContext) for now.

I think it is enough to know pinia, I will check it.

@johnsoncodehk johnsoncodehk added enhancement New feature or request and removed need info labels Mar 4, 2022
@johnsoncodehk
Copy link
Member

@zardoy seems the sorting is consistent to original response.

pinia:

螢幕截圖 2022-03-12 14 17 03

@reduxjs/toolkit:

螢幕截圖 2022-03-12 14 21 29

螢幕截圖 2022-03-12 14 21 44

@johnsoncodehk johnsoncodehk added need info and removed enhancement New feature or request labels Apr 4, 2022
@zardoy
Copy link
Contributor Author

zardoy commented Apr 28, 2022

Sorry for late reply. @johnsoncodehk, just if you're curious, I tried a simple TS plugin that solves the issue. It works well in all cases from above

import ts_module from 'typescript/lib/tsserverlibrary'

export = function ({ typescript }: { typescript: typeof ts_module }) {
    return {
        create(info: ts.server.PluginCreateInfo) {
            // Set up decorator object
            const proxy: ts.LanguageService = Object.create(null)

            for (const k of Object.keys(info.languageService)) {
                const x = info.languageService[k]!
                // @ts-expect-error - JS runtime trickery which is tricky to type tersely
                proxy[k] = (...args: Array<Record<string, unknown>>) => x.apply(info.languageService, args)
            }

            proxy.getCompletionsAtPosition = (fileName, position, options) => {
                const prior = info.languageService.getCompletionsAtPosition(fileName, position, options)
                if (!prior) return
                // Feature: Force Suggestion Sorting
                prior.entries = prior.entries.map((entry, index) => ({ ...entry, sortText: `${entry.sortText ?? ''}${index}` }))
                return prior
            }
            return proxy
        },
    }
}

Sorry I didn't really have time to figure out why it doesn't work with the latest versions of Volar. It does work with builtin TS support (4.6.3) and I hope there will be a way to use this simple plugin in Volar.

Again, I found it incredibly useful, so I think I will even publish this as extension. Will Volar read and use typescriptServerPlugins from ext?

@johnsoncodehk johnsoncodehk reopened this Apr 28, 2022
@zardoy
Copy link
Contributor Author

zardoy commented May 7, 2022

sorting is consistent to original response

I finally have had time to dig into it. If you're curious why you had it: microsoft/TypeScript#49012 (spoiler: we've had different versions of the TS Server somehow)

@johnsoncodehk
Copy link
Member

I thought about it for a long time, I think vueserver should not support tsserver plugin. The tsserver plugin relies on the tsserver context, and if we try to support it, vueserver will couple the tsserver specification.

As an alternative volar plugin is sufficient for your use case, you can implement it with the following code in volar.config.js.

module.exports = {
    plugins: [
        /**
         * @type {import('@volar/language-service').LanguageServicePlugin}
         */
        (context) => {
            if (context.typescript) {
                const getCompletionsAtPosition = context.typescript.languageService.getCompletionsAtPosition;
                context.typescript.languageService.getCompletionsAtPosition = (fileName, position, options) => {
                    const result = getCompletionsAtPosition(fileName, position, options);
                    if (result) {
                        result.entries = result.entries.sort(/* ... */);
                    }
                    return result;
                };
            }
            return {};
        },
    ],
};

@johnsoncodehk johnsoncodehk added wontfix This will not be worked on and removed enhancement New feature or request labels Jan 8, 2023
@zardoy
Copy link
Contributor Author

zardoy commented Jan 8, 2023

@johnsoncodehk can I put it into VSCode extension? I obviously want to use this in every project, putting and keeping up to date this config file in ~30 project isn't a way for me 😂. If volar doesn't support looking for this configuration from contributes point of extensions, I think it would be easy to implement (though there should be a way to read VSCode configuration then). Also, as I understand from your code, you are overriding methods of language service and this is exactly what TS plugin does. So what's the difference then?
UPD: or do you mean that methods of language service coming from vueserver and they slightly different from tsserver ls? Also I notice you're not passing formatOptions as last arg to getCompletions method

@johnsoncodehk
Copy link
Member

You can put it to a single folder like /volar-global/config.js, and add "volar.vueserver.configFilePath": "/volar-global/config.js" to your VSCode user-space settings, so it would take effect globally. We currently intentionally avoid supporting the contributions point of extensions.

There is no difference between this use case and the ts plugin, because this use case does not rely on more ts plugin contexts like serverHost, projectHost. So you can try to apply your ts plugin directly in the volar plugin.

@MichaelBitard
Copy link

We have the same problem with vue-tsc, where the plugins "typescript-plugin-css-modules" are not used by vue-tsc. Is this the same resolution?

@johnsoncodehk
Copy link
Member

@MichaelBitard TS plugin is only applicable to tsserver, it is not valid for tsc or vue-tsc TS plugin. tsc has workarounds like ttypescript, but that's out of volar's project scope.

If you want to change vue-tsc type behavior, you can try experimentalAdditionalLanguageModules (#2267), it should be enough to support css-modules use case.

@MichaelBitard
Copy link

Thanks, I did not knew that! Indeed it is in typescript documentation, plugins are for augmenting the editing experience, not for changing the compilation behavior.

@johnsoncodehk
Copy link
Member

So in fact compilerOptions.plugins is wrong in meaning. 😅

@MichaelBitard
Copy link

Yeap, that's confusing 💯

@zardoy
Copy link
Contributor Author

zardoy commented Feb 7, 2023

There is no difference between this use case and the ts plugin, because this use case does not rely on more ts plugin contexts like serverHost, projectHost.

@johnsoncodehk I actually tested it almost instantly, but completely forgot to reply here. So following what you said I successfully applied whole plugin to Volar and all features work well!

So you can try to apply your ts plugin directly in the volar plugin.

But how should I've done this? Currently I'm changing user setting volar.vueserver.configFilePath so it points to my plugin loader (it can essentially load almost any plugin), but I don't like it as it doesn't allow me to provide out of the box support as it would break user setting value (eg default config path)

Also, how its possible with context.env.configurationHost.getConfiguration to fetch language-specific settings? e.g. getConfiguration('section', 'vue') I want get settings but with ability to override them in [vue] configuration section.

We currently intentionally avoid supporting the contributions point of extensions.

But why? Why don't provide some early support for ts plugins for example in already existing typescriptServerPlugins like so:

{
    "contributes": {
        "typescriptServerPlugins": [
            {
                "name": "typescript-essential-plugins",
                "enableForWorkspaceTypeScriptVersions": true,
                "volarCompatibilityVersion": 1
            }
        ]
    }
}

I don't mind if it gets thrown away over time, its still better than nothing. typescriptServerPlugins already has properties that were removed in past. Or at least provide a way to contribute extension's volar config via experimental contribution point or api.

If its just matter of time and focus just let me know I'll try to provide my own implentation.

@johnsoncodehk
Copy link
Member

First thanks for the feedback.

The advice is just for users and not for extension authors, so volar.vueserver.configFilePath is not suitable for your use case.

Contribution points is the appropriate approach, and the reasons for avoiding it in the past are:

  1. From experience with TS plugin, it often creates more confusion.
  2. Even for the TS ecosystem, the TS plugin is a relatively niche demand. Most TS users do not understand the TS plugin, and it will be even more niche for the Vue ecosystem.
  3. This will be specific to VSCode
  4. The design of the Volar Plugin API is not yet stable.

If we're going to support Contribution Points, I hope at least volarjs.github.io has documentation for Contribution Points and the Plugin API, so it will not be something users can't address.

For the second question, have you tried passing in the uri as the second parameter? e.g. getConfiguration('section', 'file:///foo.vue').

@zardoy
Copy link
Contributor Author

zardoy commented Feb 7, 2023

Thank you so much for quick replly!

Contribution points is the appropriate approach

With all that said, it can create confusion for users but not for TS dev (I'm using it to improve editing experience only) and as you can probably see I don't really use Volar plugin API (just patching TS LS methods), and even if API isnt stable yet, I'd happy to update support (I'm already using volar.config.js anyway). I just creates confusion for me why contribution point can't be supported when loading plugin from user setting is already supported?

Anyway I'm eagarly waiting for it to be implemented, so I can stop using setting workaround. So let know when some kind of contribution point will be available (or when you have plans for it)!

I hope at least volarjs.github.io has documentation for Contribution Points and the Plugin API, so it will not be something users can't address

At very least we can add contribution point to json schema so it will be more visible for vscode developers.

For the second question, have you tried passing in the uri as the second parameter? e.g. getConfiguration('section', 'file:///foo.vue')

Thanks, but I resolved it. FYI I looked at GH code, and looks like the right way to do this: Object.entries(getConfiguration('[vue]')).filter(([key]) => key.startsWith('section.')) (though config is flattened for some reason)

@zardoy
Copy link
Contributor Author

zardoy commented Feb 14, 2023

@johnsoncodehk By the way I'm trying to change results of references result from volar.config.ts (currently overriding typescript ls method), but it seems to me that typescript.languageService.findReferences never gets called O_o.

I don't see any documentation on existing language service plugins (only https://github.com/vuejs/language-tools/tree/master/packages/vue-language-service/src/plugins), and I'm trying this:

// plugin in volar.config.js
return {
    findReferences() {
        return []
    },
}

But I'm still getting results, so as I understand plugins can only append results, but not change other results? So can I use something like middlewares?

@johnsoncodehk
Copy link
Member

johnsoncodehk commented Feb 14, 2023

@johnsoncodehk By the way I'm trying to change results of references result from volar.config.ts (currently overriding typescript ls method), but it seems to me that typescript.languageService.findReferences never gets called O_o.

It uses getReferencesAtPosition() instead of findReferences(), I'm not sure if I should use findReference() instead for now.

https://github.com/volarjs/plugins/blob/44317449f6a62fa80fab647611118a1af162cd13/packages/typescript/src/services/references.ts#L20

But I'm still getting results, so as I understand plugins can only append results, but not change other results? So can I use something like middlewares?

Yes, you're right. We don't have middleware API for now (v1.0.24).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants