Import hints not working when using export default after class definition

[mjbvz]

From @methyl on October 10, 2017 7:25

• VSCode Version: 1.17.0
• OS Version: Arch Linux

Steps to Reproduce:

1. Open a JS project with jsconfig.json having checkJs: true in compilerOptions
2. Create a test.js with following content:

   class Test {}
   export default Test

3. Create a test2.js with following content:

   class Test2 extends Test {}

Actual behavior:

• No lightbulb with hint to import Test from './test appear.

Correct behavior

• Lightbulb with hint to import Test from './test appear, just like when test.js have export default inlined:

  export default class Test {}

Copied from original issue: Microsoft/vscode#35966

[mjbvz]

Confirmed this happens with TypeScript 2.5.3 and 2.6.0-20171011

[amcasey]

isExportDefaultSymbol is returning false because the declaration has no default modifier.

[amcasey]

Related case: export { Test as default };

[amcasey]

@andy-ms, I'm told you've been working in this area and might have some thoughts on what these default export helpers should do (i.e. how to divide the fix between the compiler and the services).

[amcasey]

Here's a hackish solution to the cases we've identified so far. https://github.com/amcasey/TypeScript/tree/Scratch19115

[amcasey]

I see four other callers to getLocalSymbolForExportDefault and they basically all just want the local name of the default export. Possibly a helper returning that value is in order.

[ghost]

@amcasey That branch looks like it will work.

[amcasey]

@andy-ms so don't update the other callers?

[ghost]

• services/utilities.ts and findAllReferences.ts: Update is probably a good thing
• completions.ts: Here we actually need a Symbol and not just a name. I had a pull request #20987 relating to this, let me know if you find any more failure cases.
• checker.ts: I don't think this one is necessary to change, because we're in resolveNameHelper so we should have already found the local foo if there's export default foo;.

[amcasey]

The change in findAllReferences.ts broke some tests. I'm still investigating. The failures looked like regressions, which makes me think that we might want to go the other way and pull out the default-export handling that's already there (because it's not clear why that one form would be special).

[amcasey]

As far as I can tell, the occurrence in services/utilities.ts doesn't matter - it's only used by rename and you can't rename default.

[amcasey]

For findAllReferences, I tested all three implementations (current, special case all default exports, special case no default exports) against all three types of default exports and the current implementation had the best behavior. Presumably, this means that the other two types of default exports (i.e. not handled by getLocalSymbolForExportDefault) are handled downstream.

[methyl]

Thanks a lot @amcasey, excellent work!