Skip to content

Refactor findAllReferences. Now supports renamed exports and imports. #14001

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

Merged
16 commits merged into from
Apr 14, 2017

Conversation

ghost
Copy link

@ghost ghost commented Feb 10, 2017

Don't think there was an issue for this but was mentioned here: #13643 (comment).
Fixes #13663.
Update: Now also fixes #10894.
Update: Fixes #14103.
We now use an algorithm to search for imports of a given export. This is in the new importTracker.ts. Previously we would just globally search for identifiers with the same name.

Note: This breaks find-all-refs for untyped modules since we want to match the symbol and not just the name. See #14002.

@ghost ghost mentioned this pull request Feb 10, 2017
@ghost ghost force-pushed the refactor_findallrefs branch from 0f57e97 to 231cfbc Compare February 10, 2017 22:16
@ghost ghost requested review from vladima and mhegazy February 10, 2017 22:21
@ghost ghost force-pushed the refactor_findallrefs branch 2 times, most recently from c7f473e to 5cb2800 Compare February 13, 2017 19:01
@ghost ghost mentioned this pull request Feb 14, 2017
@ghost ghost force-pushed the refactor_findallrefs branch from 5cb2800 to 151023c Compare February 16, 2017 14:59
@mhegazy
Copy link
Contributor

mhegazy commented Mar 20, 2017

Some comments:

  • getReferencesForStringLiteral, why does it use type

  • getReferencesInContainer, consider passing in the containig sourceFile

  • searchForImportsOfExport, consider changing the switch in the IIFE to just set a variable

  • sourceFileHasName, consider inlining this at use site

  • getSymbolScope, consider making the comment more detailed for export as namespace

  • getDirectImportsMap, I would change the map to use a Map object instead

  • export assignment handling, export = foo, export default foo

  • consider renaming isProperImportEquals to isExternalModuleImportEqual

  • forEachImport, consider storing the require calls on the sourceFile like we do with the imports
    this makes forEachPossibleImportOrExportStatement jsut a walk over ambient modules

  • getImportOrExportSymbol, consider changing the return expression to return comingFromExport ? getExport() : getExport() || getImport();

  • getExport, make sure to check comingFromExport before return an import

  • isNodeImport, in case for ImportEqualsDeclaration, check for is it an import .. = require or just an import .. = X

  • nodeIsEligibleForRename, check the parent of the node to make sure it is an import or export specifier, and worth adding a test of x.default vs a case of import * as ns from "mod"; ns.default;

@mhegazy
Copy link
Contributor

mhegazy commented Mar 23, 2017

@Andy-MS can you also validate that this change addresses #14346 and

@ghost ghost force-pushed the refactor_findallrefs branch from 709483f to 28a3604 Compare March 27, 2017 21:17
@ghost
Copy link
Author

ghost commented Mar 27, 2017

@mhegazy Should all be done except for #14880, which I assigned to myself.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 28, 2017

@Andy-MS i still do not see #14346 with this change..

@ghost
Copy link
Author

ghost commented Mar 31, 2017

Should be in 28a3604

@ghost ghost merged commit 72feaad into master Apr 14, 2017
@ghost ghost deleted the refactor_findallrefs branch April 14, 2017 16:57
@ghost ghost mentioned this pull request Apr 19, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 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
2 participants