fix(42605): support refactoring for export default assignment without equal #42936

Zuckjet · 2021-02-24T12:00:17Z

The original issue pointed that Convert default export to named export refactoring is not available when it has specific format below.

export default identifier:

  const foo = () => {};
  export default foo;

anonymous function:
export default () => {}

I can confirm the first situation is a bug, since we support export default function foo () {} refactoring currently.
But for second situation, I think it may be a feature not a bug, because export default function() {} is also not supported, and I traced the original relevant PR, this situation is not supported at very beginning.

So I just fix the bug part, as for feature part, I am not quite sure.

typescript-bot · 2021-02-24T12:00:24Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

andrewbranch · 2021-03-04T18:02:47Z

What are the parser and checker changes for?

Zuckjet · 2021-03-04T23:31:15Z

What are the parser and checker changes for?

for parser, update nextTokenCanFollowDefaultKeyword() function , since we should support export default foo.
for checker, make checkGrammarModifiers() function not throw grammar error when encounter export default foo

andrewbranch · 2021-03-04T23:46:55Z

I mean, I’m asking how that’s different from the current behavior.

Zuckjet · 2021-03-05T00:07:14Z

I mean, I’m asking how that’s different from the current behavior.

this changes is for refactoring. currently select export default foo, then apply refactor, "Convert default export to named export" is not available.

this PR make this refactor available:

andrewbranch · 2021-03-05T00:25:53Z

this changes is for refactoring

Right. My point is, changing the behavior of a refactor usually doesn’t require changing the parser or checker, and conversely, changing the parser and checker will usually have a much broader impact than changing the behavior of a refactor. Clearly we already parse and check export default foo without errors, so at a glance I don’t understand why those changes were necessary.

Zuckjet · 2021-03-08T12:20:20Z

@andrewbranch Thanks for your patience and advice, I finally understand what you mean. The codebase has changed since this PR created, and I find there is no need to change checker any more, but the parser seems still need change, here is the reason:
The changeExport refactor function will find DefaultKeyword in exportNode's modifiers, then delete it:

TypeScript/src/services/refactors/convertExport.ts

Lines 108 to 111 in 4d50624

    
           function changeExport(exportingSourceFile: SourceFile, { wasDefault, exportNode, exportName }: ExportInfo, changes: textChanges.ChangeTracker, checker: TypeChecker): void { 
        
               if (wasDefault) { 
        
                   changes.delete(exportingSourceFile, Debug.checkDefined(findModifier(exportNode, SyntaxKind.DefaultKeyword), "Should find a default keyword in modifier list")); 
        
               }

For export default function foo () {}, it's NodeObject's modifiers property is like {..., modifiers:[{TokenObject}, {TokenObject}]}. But for const foo = () => {};export default foo;, it's NodeObject's modifiers property is undefined. So I changed the parser, make the NodeObject of this format has right modifiers(array of two TokenObject).

It should be noted that export default foo; is also parsed as a ExportAssignment NodeObject.This format is a little different with export = foo(the official doc described as ExportAssignment), So I have to do some extra work.

andrewbranch · 2021-03-08T18:08:01Z

Ah, yes. So, this is a weird part of our parse tree, but I think it’s working as intended. We parse export default foo and export = foo both as an ExportAssignment (which doesn’t exist in the ES grammar spec). You can tell them apart by the exportAssignment.isExportEquals property. If it’s undefined/false, it was export default foo. Having modifiers there would be redundant. It means you’ll have to handle a couple more cases in the refactor, but changing the parser would be a breaking API change, which we want to avoid.

src/compiler/checker.ts

Zuckjet · 2021-03-09T13:43:01Z

src/compiler/utilities.ts

@@ -4720,7 +4720,7 @@ namespace ts {
     * NOTE: This function does not use `parent` pointers and will not include modifiers from JSDoc.
     */
    export function getSyntacticModifierFlagsNoCache(node: Node): ModifierFlags {
-        let flags = modifiersToFlags(node.modifiers);
+        let flags = (isExportAssignment(node) && !node.isExportEquals) ? ModifierFlags.None : modifiersToFlags(node.modifiers);


We should handle this, or the Convert default export to named export options will be disabled.

Doesn’t modifiersToFlags already return None in this case?

Fixed, same mistake #42936 (comment)

Zuckjet · 2021-03-09T13:46:46Z

Ah, yes. So, this is a weird part of our parse tree, but I think it’s working as intended. We parse export default foo and export = foo both as an ExportAssignment (which doesn’t exist in the ES grammar spec). You can tell them apart by the exportAssignment.isExportEquals property. If it’s undefined/false, it was export default foo. Having modifiers there would be redundant. It means you’ll have to handle a couple more cases in the refactor, but changing the parser would be a breaking API change, which we want to avoid.

As you suggested, I have updated my PR, please help review it again.

src/services/findAllReferences.ts

src/services/refactors/convertExport.ts

andrewbranch

Sorry, GitHub was having some issues yesterday and it looks like these two comments didn’t make it through.

src/compiler/checker.ts

andrewbranch · 2021-03-23T17:20:59Z

src/compiler/utilities.ts

@@ -4720,7 +4720,7 @@ namespace ts {
     * NOTE: This function does not use `parent` pointers and will not include modifiers from JSDoc.
     */
    export function getSyntacticModifierFlagsNoCache(node: Node): ModifierFlags {
-        let flags = modifiersToFlags(node.modifiers);
+        let flags = (isExportAssignment(node) && !node.isExportEquals) ? ModifierFlags.None : modifiersToFlags(node.modifiers);


Doesn’t modifiersToFlags already return None in this case?

… equal

Zuckjet · 2021-03-23T18:38:48Z

Thanks for review, I have updated this PR again, ready for new round review.

Zuckjet · 2021-04-01T22:15:04Z

just friendly ping

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Feb 24, 2021

Zuckjet force-pushed the fix/refactoring branch from 0eef449 to 7d96672 Compare February 24, 2021 12:08

sandersn requested review from andrewbranch and sandersn March 4, 2021 15:07

sandersn assigned andrewbranch Mar 4, 2021

Zuckjet force-pushed the fix/refactoring branch from 7d96672 to 776c8f0 Compare March 8, 2021 12:21

Zuckjet force-pushed the fix/refactoring branch from 776c8f0 to d3a2556 Compare March 9, 2021 13:19

Zuckjet commented Mar 9, 2021

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

Zuckjet commented Mar 9, 2021

View reviewed changes

andrewbranch requested changes Mar 22, 2021

View reviewed changes

src/services/findAllReferences.ts Outdated Show resolved Hide resolved

src/services/refactors/convertExport.ts Outdated Show resolved Hide resolved

src/services/refactors/convertExport.ts Show resolved Hide resolved

src/services/refactors/convertExport.ts Outdated Show resolved Hide resolved

Zuckjet force-pushed the fix/refactoring branch from d3a2556 to 4e358e7 Compare March 23, 2021 17:14

andrewbranch reviewed Mar 23, 2021

View reviewed changes

Zuckjet force-pushed the fix/refactoring branch from 4e358e7 to 1f85cbc Compare March 23, 2021 17:44

fix(42605): support refactoring for export default assignment without…

7629184

… equal

Zuckjet force-pushed the fix/refactoring branch from 1f85cbc to 7629184 Compare March 23, 2021 18:26

Zuckjet changed the title ~~fix(42605): make Convert default export to named export refactoring…~~ fix(42605): support refactoring for export default assignment without equal Mar 23, 2021

Zuckjet requested a review from andrewbranch March 23, 2021 18:37

andrewbranch approved these changes Apr 5, 2021

View reviewed changes

andrewbranch merged commit f621d67 into microsoft:master Apr 5, 2021

fix(42605): support refactoring for export default assignment without equal #42936

fix(42605): support refactoring for export default assignment without equal #42936

Uh oh!

Conversation

Zuckjet commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Feb 24, 2021

Uh oh!

andrewbranch commented Mar 4, 2021

Uh oh!

Zuckjet commented Mar 4, 2021

Uh oh!

andrewbranch commented Mar 4, 2021

Uh oh!

Zuckjet commented Mar 5, 2021

Uh oh!

andrewbranch commented Mar 5, 2021

Uh oh!

Zuckjet commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewbranch commented Mar 8, 2021

Uh oh!

Uh oh!

Zuckjet Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewbranch Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Zuckjet Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Zuckjet commented Mar 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewbranch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewbranch Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Zuckjet commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zuckjet commented Apr 1, 2021

Uh oh!

Uh oh!

Zuckjet commented Feb 24, 2021 •

edited

Loading

Zuckjet commented Mar 8, 2021 •

edited

Loading

Zuckjet Mar 9, 2021 •

edited

Loading

Zuckjet commented Mar 23, 2021 •

edited

Loading