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 code actions for React #112

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Conversation

AgentRBY
Copy link

@AgentRBY AgentRBY commented Mar 10, 2023

Made 6 code actions for React.

Also, I have a couple of questions:

  1. Is it possible to set the cursor position in Code Action? I tried with $0, but that doesn't work
  2. I think we need to check if there is a React in the project before adding these Code Actions, but I haven't found a way to do that

@zardoy
Copy link
Owner

zardoy commented Mar 10, 2023

Hm, the idea is cool, but I have mixed feelings of simple code actions like wrap into condition. I was always using Surround extension for wrapping anything 🤷, on the other hand I think we can also provide these code actions when user doesn't have any selection and cursor is jsx opening tag name, so user can save some time with selecting component to wrap.
And also I have very mixed feeling of having createJSXMap, maybe it'd be better to add mapDiv completion before map instead? Completion is more logical to use if its indeded for writing new code (instead of refactoring existing). And you'd better to think wether its even worth of adding, as its just a matter of writing <div></div> after map snippet that we already have, maybe we should allow emmet completion in this location instead?

While having your code actions is fine (as long as most of them have unique activation ranges) I'd like to see another a few more unique code actions, such as code action to quickly introduce useRef variable, there is an example with Typography from mui (you can try to with <player> for the first time):

// typographyRef is undefined within component
<Typography ref={typographyRef}>404</Typography>

Quickfix to add useRef add to top (we need to place a new useRef directly after last useRef):

const A = () => {
    const typographyRef = useRef<HTMLSpanElement>(null!) // notice type inferrence
    
    return <Typography ref={typographyRef} />
}

As you noticed, it more looks like a quickfix for not defined variable, instead of refactoring...

Also what do you think of adding refactorings to infer types for useState (from set invocations) and useRef (like above, but for existing useRefs)?

Is it possible to set the cursor position in Code Action? I tried with $0, but that doesn't work

VS Code uses simple text replace, because TS doesn't support snippets in code actions: microsoft/TypeScript#50166

However, not all code actions are in codeActions/custom/, there are also code actions that require interactivity so called "two-step code actions" e.g. objectIntoArrayConverters.ts I'm going to fix it soon and put them all into one place, after that we can let other code actions use the same approach, so we can use snippets instead of plain text.

Aaand finally, I don't really like doing the same thing that already was done before (thats why I try to avoid adding refactorings from abracadabra, but not from p42 though). Most code actions you are looking for were already implemented in Typescript React hooks Tools, it also has some high-quality code. Probably I'd improve that extension instead, but only if its still maintained. I hope you do support this idea. Otherwise we can reuse most of the logic from these refactorings, but they should be improved as they don't add auto-imports, but its easy to fix. Because of that I don't recommend changing these duplicated code actions for now.

Anyway, you can try to add proposed new code actions (as they should be easy to go), feel free to ask questions about them. I'll also provide additional feedback on the auto-imports soon

@zardoy zardoy changed the title feat: Add 6 code actions for React feat: Add code actions for React Mar 10, 2023
@zardoy
Copy link
Owner

zardoy commented Mar 10, 2023

on the other hand I think we can also provide these code actions when user doesn't have any selection and cursor is jsx opening tag name, so user can save some time with selecting component to wrap.

Okay, nevermind I've heard its bad idea to display surround snippet without selection only in one specific location, WDYT? Also, only these refactorings require snippets (cursor positioning), so maybe you can it will be easier to use that Surround extension instead? What are downsides of such approach?

Here is I'm attaching my Surround snippets:
"surround.custom": {
    "renderCond": {
        "label": "renderCond",
        "snippet": "{$1 ? $TM_SELECTED_TEXT : $2}",
        "languageIds": [
            "typescriptreact",
            "javascriptreact",
            "vue"
        ]
    },
    "renderAnd": {
        "label": "renderAnd",
        "snippet": "{$1 && $TM_SELECTED_TEXT}",
        "languageIds": [
            "typescriptreact",
            "javascriptreact",
            "vue"
        ]
    },
    "renderStringCond": {
        "label": "renderStringCond",
        "snippet": "{$1 ? '$TM_SELECTED_TEXT' : $2}",
        "languageIds": [
            "typescriptreact",
            "javascriptreact",
            "vue"
        ]
    },
    "objectCond": {
        "label": "objectCond",
        "snippet": "...$1 ? {$TM_SELECTED_TEXT} : null",
        "languageIds": [
            "typescript",
            "javascript",
            "typescriptreact",
            "javascriptreact",
            "vue"
        ]
    },
    "cond": {
        "label": "cond",
        "snippet": "$1 ? $TM_SELECTED_TEXT : $2",
        "languageIds": [
            "typescript",
            "javascript",
            "typescriptreact",
            "javascriptreact",
            "vue"
        ]
    },
    "condElse": {
        "label": "condElse",
        "snippet": "$1 ? $2 : $TM_SELECTED_TEXT",
        "languageIds": [
            "typescript",
            "javascript",
            "typescriptreact",
            "javascriptreact",
            "vue"
        ]
    },
    "`": {
        "label": "`",
        "snippet": "`$1${$TM_SELECTED_TEXT}$2`",
        "languageIds": [
            "typescript",
            "javascript",
            "typescriptreact",
            "javascriptreact",
            "vue"
        ]
    },
    "lnIf": {
        "label": "lnIf",
        "snippet": "if ($1) $TM_SELECTED_TEXT",
        "languageIds": [
            "typescript",
            "javascript",
            "typescriptreact",
            "javascriptreact",
            "vue"
        ]
    },
    "describeOnly": {
        "label": "describeOnly",
        "snippet": "describe.only('$1', () => {\n\t$TM_SELECTED_TEXT\n})",
        "languageIds": [
            "typescript",
            "javascript",
        ]
    },
    "setTimeout": {
        "label": "setTimeout",
        "snippet": "setTimeout(() => {\n\t$TM_SELECTED_TEXT\n}, $1)",
        "languageIds": [
            "typescript",
            "typescriptreact",
            "javascript",
            "javascriptreact",
            "vue",
        ]
    },
    "console.time": {
        "label": "console.time",
        "snippet": "console.time('$1')\n$TM_SELECTED_TEXT\nconsole.timeEnd('$1')",
        "languageIds": [
            "typescript",
            "javascript",
            "typescriptreact",
            "javascriptreact",
            "vue"
        ]
    }
}

@AgentRBY
Copy link
Author

Okay, nevermind I've heard its bad idea to display surround snippet without selection only in one specific location, WDYT? Also, only these refactorings require snippets (cursor positioning), so maybe you can it will be easier to use that Surround extension instead? What are downsides of such approach?

Well, I don't really like that Surround uses the command palette instead of CodeActions. Also, to use Surround you need to select the text, and if you have a large nesting, selecting it may take more time than just using Code Action on the right tag.

The only thing I would do in our case is move them to the Surround category instead of Rewrite. Although, like you said, it's bad practice to use Surround Code Actions without highlighting, so I'm not sure.

By the way, some of these Code Actions were inspired by React Buddy for WebSotrm. There this refactoring is implemented through both WebSotrm's built-in "Wrap with" and Code Actions.

Also, only these refactorings require snippets (cursor positioning)

In general, not just there. For example, for createPropsInterface it would be nice to move cursor inside interface. Or for generateJSXMap replace div with something else (which I'll write more about below)

And also I have very mixed feeling of having createJSXMap, maybe it'd be better to add mapDiv completion before map instead? Completion is more logical to use if its indeded for writing new code (instead of refactoring existing). And you'd better to think wether its even worth of adding, as its just a matter of writing

after map snippet that we already have, maybe we should allow emmet completion in this location instead?

I originally wanted the user to enter the tags themselves, but since it's not a snippet, I just made a div. In general, the idea of adding as completion is a good one, but I wouldn't include it by default, since user expect array methods after the dot, not any snippets. Anyway, Code Action for this would still be useful, but what to do with div I don't know yet. If we make it as a snippet, we could add renaming the div to what the user needs.

While having your code actions is fine (as long as most of them have unique activation ranges) I'd like to see another a few more unique code actions, such as code action to quickly introduce useRef variable, there is an example with Typography from mui (you can try to with for the first time):
...
Also what do you think of adding refactorings to infer types for useState (from set invocations) and useRef (like above, but for existing useRefs)?

In general, I plan to do a few more Code Actions from the plugin, which I mentioned above (for example, generating event-handlers, adding props, and your suggestion with useRef is also there) and a few more of those that I have in my head. I would also implement the Code Actions you suggested. All this I would like to implement, but in next Pull Requests, that would somehow divide work and not to do all at once.

Perhaps we should create an issue listing all the Code Actions for React that should be in the extension (including the ones already implemented and the ones I named above).

Aaand finally, I don't really like doing the same thing that already was done before (thats why I try to avoid adding refactorings from abracadabra, but not from p42 though). Most code actions you are looking for were already implemented in Typescript React hooks Tools, it also has some high-quality code. Probably I'd improve that extension instead, but only if its still maintained. I hope you do support this idea. Otherwise we can reuse most of the logic from these refactorings, but they should be improved as they don't add auto-imports, but its easy to fix. Because of that I don't recommend changing these duplicated code actions for now.

First, Typescript React hooks Tools contains only two Code Actions, of which I implemented only one (namely "Wrap with useCallback"). I did not implement "Wrap with useMemo", because I have not yet figured out when to show it, so as not to be annoying.

Second, Typescript React hooks Tools uses selection to show the action. I, on the other hand, show the action when the cursor is inside the variable (which makes more sense to me). Maybe I should also add support for normal functions (function test() {...}), and also that the action is shown when the cursor stands on an arrow identifier (=>), as a nice addition.

Third, it is unlikely that the developer will support this extension, the last changes were there more than two years ago. Let's wait a couple of days and see if he responded to your issue. In the meantime I think I can take some practices from his code to improve my Code Actions (like dependency detection). If the developer agrees to accept the PR, I can remove "Wrap with useCallback" from here and improve that extension.

And finally: I agree that it is better not to create what has already been done, but in this case it would be better to leave these Code Actions in this extension, because that plugin is not particularly supported, and in our extension we can fix or improve them at any time. Also, instead of the user would install an additional extension, he could use the Code Actions in our plugin. Also, other plugins could use a less productive solution for Code Actions (not in this case, but I just wanted to mention that)

@zardoy
Copy link
Owner

zardoy commented Mar 12, 2023

Well, I don't really like that Surround uses the command palette instead of CodeActions. Also, to use Surround you need to select the text, and if you have a large nesting, selecting it may take more time than just using Code Action on the right tag.

Remember that you can use Expand Selection command that uses TS knowledge and saves a lot of time. On the other hand I see your point here. It is much faster to do a quick selection with mouse and apply code action.

In general, the idea of adding as completion is a good one, but I wouldn't include it by default, since user expect array methods after the dot, not any snippets.

But there is already arrayMethodsSnippets feature which is disabled by default (you also copied part of functionality from it). Anyway, try to not add unneeded extra complexity to plugin, if you in doubt wether functionality should be added or not, don't do it, we should have strong demand for it. I try to only include functionality for things that don't have other ways to do them. And IMO in this case, if we have completion why would you need code action and vice versa.

In general, I plan to do a few more Code Actions from the plugin

Yeah, but I want to know for what cases you are also going to include code actions (such as event handler generation)? And, by the way will you be interested in doing non-react interactive code actions in future as well?

Perhaps we should create an issue listing all the Code Actions for React that should be in the extension (including the ones already implemented and the ones I named above).

This is unnecessary for code actions that are you're adding here, but you can create issue for code actions that are you not going to include in this PR in order we don't forget about them (and for easier duscission as I said above).

Also, instead of the user would install an additional extension, he could use the Code Actions in our plugin.

Generally I do not support this idea at all, I work with more than 100 extensions installed and never try to copy existing functionality, I'd better to have similar extensions links in readme (just saying)


Also Typescript React hooks Tools has npm extension and I wonder we can reuse it for our code actions

@AgentRBY
Copy link
Author

But there is already arrayMethodsSnippets feature which is disabled by default (you also copied part of functionality from it). Anyway, try to not add unneeded extra complexity to plugin, if you in doubt wether functionality should be added or not, don't do it, we should have strong demand for it. I try to only include functionality for things that don't have other ways to do them. And IMO in this case, if we have completion why would you need code action and vice versa.

I removed the generateJSXMap Code Action. Perhaps in the future I will implement completion for this, but for now I think it's better to do without it

Yeah, but I want to know for what cases you are also going to include code actions (such as event handler generation)? And, by the way will you be interested in doing non-react interactive code actions in future as well?

I don't mind in general, but first I would prefer to do all the Code Actions for react that I have planned

This is unnecessary for code actions that are you're adding here, but you can create issue for code actions that are you not going to include in this PR in order we don't forget about them (and for easier duscission as I said above).

Created #115, check if everything is ok, maybe you want to add something else


Also Typescript React hooks Tools has npm extension and I wonder we can reuse it for our code actions

Unfortunately, the npm package does not provide any API, it just allows you to initialize the plugin. Therefore, it is not possible to access internal functions and methods

@zardoy
Copy link
Owner

zardoy commented Mar 14, 2023

Hey, @AgentRBY I pushed a few changes:

  • now we support code actions with snippet, but I didn't update code actions, just merged develop
  • showed you how its possible to access api, so wrap into memo now reuses, I think its much easier to just adjust range and output (to add additional text edit for auto-import) rather to copy full implementation

Also could you change React folder name to just react. I use lowercase naming convention generally

@zardoy
Copy link
Owner

zardoy commented Mar 14, 2023

I think all code actions except wrap into memo and useCallback should use snippet edit, see declareMissingProperties.ts for example

@zardoy
Copy link
Owner

zardoy commented Mar 18, 2023

Would you be up for cleaning up wrap memo & callback actions? (if you like the idea I showed in useMemo action)

ALso I'm currently thinking of how its possible to reuse builtin logic for adding auto-imports edits

@AgentRBY
Copy link
Author

Would you be up for cleaning up wrap memo & callback actions? (if you like the idea I showed in useMemo action)

ALso I'm currently thinking of how its possible to reuse builtin logic for adding auto-imports edits

I've been busy the last couple of days, I'll try to take a look today

@AgentRBY
Copy link
Author

Hi @zardoy, if you can complete this PR it will be very good, I will be busy soon, so there is not much time for it, sorry

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.

2 participants