Skip to content
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

Do not trust Scope when removing TypeScript types #10174

Merged
merged 5 commits into from Jul 8, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -19,6 +19,16 @@ function isInType(path) {
}

const PARSED_PARAMS = new WeakSet();
const GLOBAL_TYPES = new WeakMap();

function isGlobalType(path, name) {
const program = path.find(path => path.isProgram()).node;
return GLOBAL_TYPES.get(program).has(name) && !path.scope.hasOwnBinding(name);
}

function registerGlobalType(programScope, name) {
GLOBAL_TYPES.get(programScope.path.node).add(name);
}

export default declare(
(api, { jsxPragma = "React", allowNamespaces = false }) => {
@@ -40,6 +50,10 @@ export default declare(
const { file } = state;
let fileJsxPragma = null;

if (!GLOBAL_TYPES.has(path.node)) {
GLOBAL_TYPES.set(path.node, new Set());
}

if (file.ast.comments) {
for (const comment of (file.ast.comments: Array<Object>)) {
const jsxMatches = JSX_ANNOTATION_REGEX.exec(comment.value);
@@ -50,7 +64,7 @@ export default declare(
}

// remove type imports
for (const stmt of path.get("body")) {
for (let stmt of path.get("body")) {
if (t.isImportDeclaration(stmt)) {
// Note: this will allow both `import { } from "m"` and `import "m";`.
// In TypeScript, the former would be elided.
@@ -91,6 +105,28 @@ export default declare(
importPath.remove();
}
}

continue;
}

if (stmt.isExportDeclaration()) {
stmt = stmt.get("declaration");
}

if (stmt.isVariableDeclaration({ declare: true })) {
for (const name of Object.keys(stmt.getBindingIdentifiers())) {
registerGlobalType(path.scope, name);
}
} else if (
stmt.isTSTypeAliasDeclaration() ||
stmt.isTSDeclareFunction() ||

This comment has been minimized.

Copy link
@JLHwung

JLHwung Jul 7, 2019

Contributor

Would stmd.node.id.name bail on export default function() {}?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jul 7, 2019

Author Member

TSDeclareFunction (declare function f(): type;) can't be used in an export default declaration, so it can never be anonymous.

stmt.isTSInterfaceDeclaration() ||
stmt.isClassDeclaration({ declare: true }) ||
stmt.isTSEnumDeclaration({ declare: true }) ||
(stmt.isTSModuleDeclaration({ declare: true }) &&
stmt.get("id").isIdentifier())
) {
registerGlobalType(path.scope, stmt.node.id.name);
}
}
},
@@ -100,8 +136,8 @@ export default declare(
if (
!path.node.source &&
path.node.specifiers.length > 0 &&
!path.node.specifiers.find(exportSpecifier =>
path.scope.hasOwnBinding(exportSpecifier.local.name),
path.node.specifiers.every(({ local }) =>
isGlobalType(path, local.name),
)
) {
path.remove();
@@ -110,10 +146,7 @@ export default declare(

ExportSpecifier(path) {
// remove type exports
if (
!path.parent.source &&
!path.scope.hasOwnBinding(path.node.local.name)
) {
if (!path.parent.source && isGlobalType(path, path.node.local.name)) {
path.remove();
}
},
@@ -122,7 +155,7 @@ export default declare(
// remove whole declaration if it's exporting a TS type
if (
t.isIdentifier(path.node.declaration) &&
!path.scope.hasOwnBinding(path.node.declaration.name)
isGlobalType(path, path.node.declaration.name)
) {
path.remove();
}
@@ -137,7 +170,9 @@ export default declare(
},

VariableDeclaration(path) {
if (path.node.declare) path.remove();
if (path.node.declare) {
path.remove();
}
},

VariableDeclarator({ node }) {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.