Skip to content

feat: add a codefix to fix class to className in react & add spelling suggest for JSX attributes #37907

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

Merged
merged 15 commits into from
Jun 23, 2020

Conversation

Jack-Works
Copy link
Contributor

Add a code fix to fix class in JSX (to className) or for in JSX (to htmlFor). This is useful for copy-paste HTML code into a JSX file.

Before:
image

image

After (fix all):
image

@sandersn sandersn added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 5, 2020
@sandersn
Copy link
Member

sandersn commented May 5, 2020

I don't know how JSX name resolution works here, but I'm surprised that it's a type assignability error instead of a name resolution error. If it were the latter, you could probably just special-case the spelling correction algorithm and use the existing did-you-mean codefix.

Switching which error is issued actually might be the right fix for this.

@sandersn sandersn assigned orta and unassigned andrewbranch May 5, 2020
@orta
Copy link
Contributor

orta commented May 5, 2020

I manually do this all the time!

@Jack-Works
Copy link
Contributor Author

Jack-Works commented May 6, 2020

If it were the latter, you could probably just special-case the spelling correction algorithm and use the existing did-you-mean codefix.

Yes, in my last try I tried to use it but it seems like JSX never emit diag 2551 (... not exist on type ..., do you mean ...?)

// The following 2 property both misspelled with doubled last letter
const x = <a onCompositionUpdateCapturee={() => {}} />;
// Property '...' does not exist on type '...'.ts(2322)
window.AbortControllerr
// Property '...' does not exist on type '...'. Did you mean '...'?ts(2551)

I'll try to investigate this again

@Jack-Works
Copy link
Contributor Author

Oh I found a comment:

https://github.com/microsoft/TypeScript/blob/c219fdae08607e07ec875d2137fcba7c2d93792c/src/compiler/checker.ts#L15984
TODO: Spelling suggestions for excess jsx attributes (needs new diagnostic messages)

🤔 let me try it

@Jack-Works
Copy link
Contributor Author

Jack-Works commented May 6, 2020

image

OK it's working now

TODO

  • Add test for this change

@Jack-Works Jack-Works force-pushed the feat/class-to-classname branch from 1513326 to 769c52c Compare May 6, 2020 14:54
@Jack-Works
Copy link
Contributor Author

Jack-Works commented May 6, 2020

image

@andrewbranch
I have re-implemented this with spelling checking logic. It's working well on the InstinctElements but the code fix not working well on class components because the suggestion is wrapped in a "No overload matches this call." diag therefore the fixSpelling doesn't recognize that. Resolved by changing fixSpelling.

image

@Jack-Works
Copy link
Contributor Author

Done. I'll add some test for it later

@Jack-Works
Copy link
Contributor Author

Test added. ready for review

@Jack-Works Jack-Works changed the title feat: add a codefix to fix class to className in react feat: add a codefix to fix class to className in react & add spelling suggest for JSX attributes May 7, 2020
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌟

@sandersn
Copy link
Member

@sheetalkamat are you OK with this change now? @andrewbranch likes it at least =)

@sandersn
Copy link
Member

sandersn commented Jun 18, 2020

It's not a spelling mistake, it is another type of mistake.

I think it's cross-language interference. This is like an editor that lets you paste in Norwegian and offers to translate it to Swedish.

@Jack-Works Jack-Works force-pushed the feat/class-to-classname branch from 78c5f67 to 4fce495 Compare June 19, 2020 03:28
@Jack-Works Jack-Works requested a review from sandersn June 19, 2020 03:29
const propName = symbolToString(prop);
const suggestionSymbol = getSuggestedSymbolForNonexistentJSXAttribute(propName, errorTarget);
const suggestion = suggestionSymbol ? symbolToString(suggestionSymbol) : undefined;
if (suggestion) reportError(Diagnostics.Property_0_does_not_exist_on_type_1_Did_you_mean_2, propName, typeToString(errorTarget), suggestion);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's technically the possibility of an empty symbol name here, but not clear when that comes up.

Also, as a nit, whenever we have an if/else pair we generally wrap those in curlies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how to handle a symbol without name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (suggestion !== undefined)

@DanielRosenwasser DanielRosenwasser merged commit e6aedfd into microsoft:master Jun 23, 2020
@DanielRosenwasser
Copy link
Member

Thanks @Jack-Works!

@Jack-Works Jack-Works deleted the feat/class-to-classname branch April 23, 2022 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants