Closed domenic closed 5 years ago
I've pushed an additional commit which shows what switching entirely to this infrastructure looks like. In that world we stop using the jscodeshift API entirely. In that case jscodeshift becomes a pretty large dependency for something that largely just wraps the read/write process, so when this project approaches "finished" we might want to consider removing it. (But for now I think it's enough of a help to keep around.)
I'll also note that https://ts-ast-viewer.com/ is helpful for seeing the world the way ts-morph sees it.
Further notes. As expected, TypeScript doesn't do global type inference. So if you try
function foo(x) {
x.log();
}
foo(console);
in the AST viewer, the x
in x.log
will be noted as having type any
. Not great.
However, in JS mode (use the options menu on the right of the AST viewer; this PR is in JS mode) it respects JSDoc comments. So given
/**
* @param {Console} x
*/
function foo(x) {
x.log();
}
foo(console);
it correctly figures out that x
in x.log
is a Console
instance (and even finds the built-in type definition for it).
More on type-checking in JS files at https://www.typescriptlang.org/docs/handbook/type-checking-javascript-files.html
So it sounds like we can create a really robust system using TypeScript, as long as we make sure we're using JSdoc comments properly
Closing as this has been incorporated into the main repo!
This pull request is some hacked-together, do-not-merge code showing a technique that uses TypeScript and ts-morph to do the console removal.
In particular, instead of just looking if the base of the call expression is an identifier named "console", it looks at the TypeScript-compiler-determined type of the expression, and checks if that type is named "Console". You can see how this helps with the edge cases, such as #12, in the newly-added test.
Things to note:
This fails if we have our own types with the same name as globals, because we're just checking
.getType().getText()
. One idea I found for fixing this is to check.getType().compilerType.symbol.getDeclarations()[0].getSourceFile().path
and seeing if it's one of the built-in TypeScript definitions. However, the built-in TypeScript definitions are not comprehensive, especially with newer or experimental specs, so I am not sure that we could completely rely on this. It's probably pretty good though---e.g. it's probably as good as any ESLint rule, so this may be good enough for #12.We could potentially use "has a built-in TypeScript type definition" as a way of determining whether it has an original. But again, since they're not comprehensive, I think the technique in #11 (of using crawl.json in particular) will probably be better.
This bypasses some of the nice work that Recast does for us, of parsing the tree, letting various transforms act on it, then re-serialized it. Instead this PR serializes the recast result, re-parses it using the TypeScript compiler, then re-serializes that using the TypeScript serializer. This is kind of sad. There may be better ways to mesh the systems---e.g., use Recast's TypeScript compiler mode, then feed the result to ts-morph?
ts-morph is a lot nicer than using the raw TypeScript compiler API. However it's still not quite as pleasant as Recast.
TypeScript's serializer makes some opinionated formatting choices; see the output file diffs. Whatever. We can always run prettier or eslint --fix later if we care.
This is a decent bit slower. The tests take 10 seconds to run vs. 3 seconds with the Recast infrastructure. There are probably things I'm doing in how I'm using the ts-morph API that make it slower (e.g. the parse-serialize-parse-manipulate-serialize strategy), but it may just be that the TypeScript compiler is slower, which would be believable.
TypeScript doesn't yet support private fields: https://github.com/microsoft/TypeScript/issues/9950. But it looks like it's coming soon??
What to do with this?
I think we are going to need this technique (in particular, need an AST with type information) for replacing method calls/getters/setters. So we're going to need this infrastructure at some point.
We could either: