Skip to content

Shrink span for convert-to-es6-module suggestion#23441

Merged
1 commit merged into
masterfrom
getErrorNodeFromCommonJsIndicator
Apr 16, 2018
Merged

Shrink span for convert-to-es6-module suggestion#23441
1 commit merged into
masterfrom
getErrorNodeFromCommonJsIndicator

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Apr 16, 2018

Fixes #23272

@ghost ghost requested a review from amcasey April 16, 2018 19:49
Copy link
Copy Markdown
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Did it used to be large because it was convenient when it was a refactoring? Or was it just an oversight?

// @Filename: /a.js
////[|exports.f = function() {}|];
////[|exports.f|] = function() {};
////exports.C = class {};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we only offer the fix on the first one?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, only at the single location that comes from commonJsModuleIndicator.

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 16, 2018

When it was a refactoring we used completely different code, since we started with a range and had to determine whether it was highlighting a commonjs-y thing. Now we are walking through the source file and adding suggestion diagnostics.
So to answer your question, there was only an error span when this was recently converted to a suggestion diagnostic instead of a refactoring.

@ghost ghost merged commit 40fd6ae into master Apr 16, 2018
@ghost ghost deleted the getErrorNodeFromCommonJsIndicator branch April 16, 2018 23:46
@microsoft microsoft locked and limited conversation to collaborators Jul 26, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant