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

Fix/22473 #27970

Closed
wants to merge 7 commits into from
Closed

Fix/22473 #27970

wants to merge 7 commits into from

Conversation

muradkhan101
Copy link

Adds a codefix for the scenarios where a user attempts to re-assign a variable that was declared as const or property that was declared readonly. In the former, it changes the initial declaration to type let and in the latter, it removes the readonly modifier.

Added two more diagnostic messages as well since, the existing ones couldn't describe the changes in a localizable way

Fixes #22473

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Also needs a codeFixAll test

@@ -4683,5 +4683,13 @@
"Generate types for all packages without types": {
"category": "Message",
"code": 95068
},
"Make all const or readonly expressions reassignable": {
Copy link

Choose a reason for hiding this comment

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

This sounds like it will change every const to let regardless of whether there's an error. Maybe this should be something like Make variables and properties mutable where necessary.

function getDeclaration(sourceFile: SourceFile, pos: number, checker: TypeChecker): Declaration | undefined {
const node = getTokenAtPosition(sourceFile, pos);
const symbol = checker.getSymbolAtLocation(node);
if (symbol) {
Copy link

Choose a reason for hiding this comment

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

Should this just be Debug.assertDefined(symbol).valueDeclaration? If we issued this error it seems like there must be a symbol?

@@ -0,0 +1,11 @@
/// <reference path='fourslash.ts' />
Copy link

Choose a reason for hiding this comment

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

This file should be a .ts file, not .tsx

changes.replaceNode(sourceFile, oldDeclarationList, declarationList);
}
else if (node.kind === SyntaxKind.PropertyDeclaration) {
const readonlyToken = findAnyChildOfKind(node, SyntaxKind.ReadonlyKeyword);
Copy link

Choose a reason for hiding this comment

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

changes.deleteModifier(sourceFile, Debug.assertDefined(findModifier(node, SyntaxKind.ReadonlyKeyword)));

@ghost ghost requested a review from DanielRosenwasser November 1, 2018 22:56
@ghost
Copy link

ghost commented Nov 2, 2018

We should avoid changing declarations that appear in node_modules or lib. I think you can use program.isSourceFileFromExternalLibrary and program.isSourceFileDefaultLibrary for that.

@muradkhan101
Copy link
Author

@DanielRosenwasser Updated the PR according to the comments made by @andy-ms

@ghost
Copy link

ghost commented Nov 12, 2018

@muradkhan101 Looks like the latest commit accidentally made a big change to diagnosticMessages.json.

@muradkhan101
Copy link
Author

@andy-ms Made the time and resolved the merge conflicts with the branch, sorry for the wait

@muradkhan101
Copy link
Author

@andy-ms This PR's been sitting for a few months now with no activity by the reviewer. Anything I need to do to move it along and get it merged?

@ghost
Copy link

ghost commented Jan 26, 2019

I'm on a different team now -- @DanielRosenwasser @sandersn @sheetalkamat Please review

@jimbuck
Copy link

jimbuck commented Jul 19, 2019

@muradkhan101 Any particular reason why you closed this PR?

This pull request was closed.
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.

Codefix: convert const to let
3 participants