path-visitor implementation origin story

[ElonVolo]

I'm trying to refactor path-visitor into something that might better meet my own needs for a particular situation.

I noticed that while ast-types is written in Typescript, the code in path-visitor.ts is written with lots of the Javscript prototype inheritance/manipulation that you'd often see people doing in Javascript prior to when Typescript got popular.

Would @benjamn, @brieb, or someone else know what the reason for implementing path-visitor.ts this way? This is not a criticism, I'm just trying to understand whether if I re-implemented path-visitor.ts in pure typescript whether I'd be leaving out some key implementation requirement I didn't think of.

Any light anyone could shed on this would be greatly appreciate.

[ElonVolo]

Right now I'm making the assumption that I can take all the path/node etc stuff and re-write it as simple Typescript classes, use inheritance, etc and that the prototype hacking is some artifact from when something was entirely javascript-based in pre-typescript days.

Am I making the wrong assumption? Is there some mistake I'd be making by gutting the prototype manipulation and refactoring to plain vanilla Typescript classes?

[pionxzh]

@ElonVolo I'm also interested in this topic. For now, I have to fork ast-types to fix issues that have existed for years. The scope analyzing is also problematic for ES6 let/const scoping.

I would be willing to work on it if there is a repo that I can join.

[ElonVolo]

As I've looked into the code, it gets more bizarre (warning, code archaeology ahead)

• Half the time the code attempts strict TS rigor. The other half the time it's like they're sprinkling any's everywhere just to get stuff to go through.

• In a similar vein, the folks writing the code try to express everything in Typescript interfaces to maximize abstraction, then it's like they suddenly realize the instanceof doesn't work with interfaces and they had to quickly splice js prototype constructors onto the interfaces just to allow the code to figure out where it is in the traversal process. So you have both ast-types and (I believe) jscodeshift accessing what should otherwise be private directories ofcode in ast-types just to get detecting of object types to work.

• It's hard for to figure tell whether much of @benjamn's and @brieb's reimplementations reflect a strongly opinionated approach versus some external force (PM, EM, project deadlines, etc) making then rush and putting pressure on them.

Whatever the motives and motivations it seems like whole design approach has resulted in Typescript that doesn't "percolate" all the way up to layers I need for my jscodeshift rewrite. I have to find a sustainable solution that doesn't sweep tech debt under the carpet another 10 years.

Anyhow, I'd be interested in collaborating on a fork with you. I'm actually working on a fork right now where I detangle a bunch of ast-types code and re-write them in plain vanilla (sub)classes. It would definitely help if I had an extra set of eyes to catch mistakes I'll probably make converting them from js prototype inheritance graphs into simple TS class hierarchies.

Caveats:

• My hard boundaries on my fork is doing everything OOP as much as possible.

• I'm writing the fork to be debugger friendly. More lines, more step-over points, no return tail-calls, no nesting statements/calls/whatever in function parameters, array indices. I want code that looks so childishly simple and easy-to-understand that you'd never get promoted for it at a FAANG.

• ast-types can be forked for now to meet our needs, but eventually the entire repo needs to be rebuilt to support linting, tsconfig, esm friendliness, jest, etc. Any fork I'd release should not be relied on for more than 2-3 years and anything that makes use of it after that time period might need some refactoring to support future versions.

I'm Toying with the idea of making a dual license where it's free for most users but FAANGS, Microsoft, and AirBnB would have a yearly licensing fee. Ben would get a cut of course. Not saying I will necessarily do it, but it's tempting. I think it's reasonable to expect the library's biggest corporate users to spend five hundred millionths of their market cap on a technology that saves them combined millions every year. I'm tempted. We'll see.

[pionxzh]

I basically cherry-pick useful PRs in this repo and merge them manually. Fix the Typescript public interface (not the internal, it's a black hole!). The biggest one I have done recently is to fix the block scoping issue (https://github.com/benjamn/ast-types/issues/154), it's very troublesome to understand and debug the scope scanning. 😪

Here is the repo: https://github.com/pionxzh/ast-types-x

I'm not a language expert but I think I can help modernize the repo / TS and some of the design.

[benjamn]

@ElonVolo I owe you a longer response, but the short-ish answer is that TypeScript struggles with the fork() system (introduced long ago in #145, when this was all just JS) that allows multiple distinct sets of types to be instantiated simultaneously/independently.

The Path class can probably be translated to an ordinary TS class, because it doesn’t have (m)any dependencies, but you can’t get much further than that, because (for example) the NodePath class depends on the definition of a Node, whose type depends on the specific type system fork you’re using.

At the time when this repo was converted to TypeScript, there did not seem to be a way to get the TS type system to understand the types of classes defined dynamically and returned/exported as values (e.g. the way the Path class is defined/returned by the pathPlugin function). Instead, TS expects classes to be defined in top-level/module scope, where they can’t easily refer to dynamic values like var types = fork.use(typesPlugin);.

You can enforce some type safety by casting the dynamic classes with code like as any as PathConstructor, though that’s obviously pretty ugly and manual. That’s the compromise we settled on, to preserve the existing fork system. Trust me when I say we didn’t just accidentally write the code that way, ignoring that classes exist.

I’m describing this situation not to justify it, but (hopefully) to save you from going through the same exercise and getting stuck. With the benefit of hindsight, I think the dynamic forking system simply isn’t compatible with a static type system like TypeScript, and if I had to choose between an idiomatic TS codebase and one capable of supporting this dynamic forking capability, I would remove the forking system and let the codebase define only one type system at a time. You could then fork the repo in the more common (Git) sense of the term if you wanted to use a different set of AST types in your project.

There’s a chance that TypeScript has gotten smarter in recent years, and these problems might no longer be so tricky. That would be a nice outcome. If not, I think the best solution is probably to release a new major version that is no longer capable of dynamic forking.

As for all the any types, that’s a remnant of an incomplete translation to TypeScript. I’m sure many of those types could be improved, though some are implementation details that wouldn’t affect usage of the library.

Long story short, if you do decide to fork the library in the Git sense, you should probably invest the time necessary to remove the fork()-based plugin system, since that system is the root cause of all the awkward interface casting shenanigans. No hard feelings if that’s the path you choose!

Alternatively, I’d be happy to pursue this line of development here, in this repo. I don’t want to be the sole gatekeeper of those changes, so it probably makes sense for me to share maintainer privileges with you and anyone else lending a hand.

I guess that wasn’t so short, but I hope it conveys the nature of the problem well enough to let you make your own decisions.