p42ai / js-assistant

120+ refactorings and code-assists for Visual Studio Code
https://marketplace.visualstudio.com/items?itemName=p42ai.refactor
MIT License
119 stars 7 forks source link

Duplicated Add Braces To Arrow Function #20

Closed hediet closed 2 years ago

hediet commented 2 years ago

image

One is from p42, the other from TypeScript

lgrammel commented 2 years ago

There is some overlap between the refactorings that P42 offers and the refactorings that are built into TypeScript.

What UX would you ideally expect here?

hediet commented 2 years ago

You can implement a typescript language service plugin to disable the TypeScript refactoring.

I would expect to not suggest the same refactoring twice.

lgrammel commented 2 years ago

I agree, seeing the same refactoring twice is not great. I'm a bit hesitant to disable functionality provided by TS or to affect their language service from the P42 extension (which can cause stability issues, especially when they upgrade to different versions).

Maybe I'll implement it as a separate open source companion extension where you can selectively disable some TypeScript refactorings. Would this be an option for you?

hediet commented 2 years ago

I would ship it with the same extension. You can still add settings to disable it.

The default experience should be a good one, seeing the same refactoring twice is not good.

If you can't or don't want to disable the TypeScript "Add braces to arrow function", you could disable P42's refactoring "Add {...} to arrow function" instead.

lgrammel commented 2 years ago

I'll think about how to best do this, maybe there are other options as well. Disabling the P42 refactoring is safer, it might be a preferrable default option.

lgrammel commented 2 years ago

Fixed in v1.112.3 for add / remove braces to arrow functions. The refactoring from the typescript language service is now hidden, except on web (because typescript does not support plugins on web yet). Thanks for the bug report!

OliverJAsh commented 2 years ago

The refactoring from the typescript language service is now hidden

Is there a way to disable this? I have the following keyboard shortcuts setup to use these built-in refactorings and they no longer work:

    {
        "key": "shift+alt+r",
        "command": "editor.action.codeAction",
        "args": { "kind": "refactor.rewrite.arrow.braces.remove" }
    },
    {
        "key": "shift+alt+a",
        "command": "editor.action.codeAction",
        "args": { "kind": "refactor.rewrite.arrow.braces.add" }
    },
image

Alternatively, is it possible to update my keyboard shortcuts to use the refactorings provided by this extension?

lgrammel commented 2 years ago

@OliverJAsh thanks for the bug report! Right now, both options are unfortunately not possible. I'll look into adding a setting for disabling the ts code action hiding, and also expose each refactoring with a specific kind, so it can be mapped to a shortcut.

lgrammel commented 2 years ago

@OliverJAsh I've just released v1.115.1. Starting with that version, you can assign keyboard shortcuts to the matching code actions from the JavaScript Assistant:

  {
    "key": "shift+alt+r",
    "command": "editor.action.codeAction",
    "args": { "kind": "refactor.convert.p42.remove-braces-from-arrow-function" }
  },
  {
    "key": "shift+alt+a",
    "command": "editor.action.codeAction",
    "args": { "kind": "refactor.convert.p42.add-braces-to-arrow-function" }
  },

Adding a setting to enable/disable the overlapping typescript refactorings and making this an opt-in experience (because it can affect the current setup) will come in a future release.

Thanks for reporting the issue!

lgrammel commented 2 years ago

@OliverJAsh I've released v1.118.0 which allows you to choose what code assists are shown when there is an overlap:

image

I've chosen the opt-out solution, i.e. P42 deactivates overlapping TS code actions by default, to provide a good user experience out of the box for most users.