Referencing a hoisted token before it's declared adds a require

[kiliancs]

A type import is correctly ignored if a runtime value is declared in the file, but this is not true if the same token is referenced before its runtime declaration. For hoisted variables like const, let and function, even in this case the require shouldn't be added.

In the following example Summary from './interface' is a type. Sucrase knows not to import it when it finds the same token in the const declaration (const Summary = 'x'). But if we place the same token Summary before the const declaration, Sucrase doesn't realize this is a reference to the const and adds a require to ./interface.

import { Summary } from './interface';

// uncommenting this adds a require('./interface').
// Summary

const Summary = 'x';

https://sucrase.io/#compareWithTypeScript=true&code=import%20%7B%20Summary%20%7D%20from%20'.%2Finterface'%3B%0A%0ASummary%0A%0Aconst%20Summary%20%3D%20'x'%3B

I realize this is not necessarily easy to fix, as the following should still add the require to ./interface:

import { Summary } from './interface';

Summary

function x() {
  const Summary = 'x';
}

But I thought filing an issue would still be a valid and useful contribution.

Edit: probably good to mention a couple workarounds:

• renaming either the type or the hoisted variable
• placing the variable declaration before other references to it

[alangpierce]

Thanks for reporting. I think it actually doesn't depend on order: we keep the import if Summary is ever accessed as a value, and ignore the import if Summary is never accessed as a value. (Usually, when it's never accessed as a value, that means it's instead accessed only as a type.) Currently we don't look at declarations, just accesses.

But actually, I think the right behavior is to say that if Summary is defined by an import statement and also declared as a variable/function/etc at the top level, then it must be that the import is for a type (otherwise it would be a duplicate declaration), so we should elide that import. I think that should be doable to implement and would fix all of the issues here. There's already some code to detect top-level declarations for the react-hot-loader transform, so I think we can use that.