why are the ESTree types distinct from ESTree? #413

43081j · 2019-04-06T20:17:29Z

For some reason, when you refactored to typescript everywhere, the choice was made to write an entire set of unconventional AST types rather than extending ESTree/ESLint types.

Out of interest, why was this?

This leads to all sorts of weirdness when mixing ESLint types in. You get away with it here because you literally rewrote all of ESLint's types and use those instead (from a pseudo-module?).

I do see the comments at the top of that file, too. But avoiding compromising type compatibility with ESLint entirely seems like it should have outweighed your need for "better control".

It means we can't context.report a "TSESTree" node if we're strictly implementing ESLint's types as we should (rather than your cloned types)
It means a typescript-eslint RuleModule isn't an ESLint RuleModule
Your node types use an enum but really should use constant string types for consistency with ESTree (example vs yours. this is pretty unconventional/inconsistent, especially since some of your types even use string constants rather than the enum)
We can't really make plugins which use typescript-eslint nodes because your types are internal? (your cloned types, i.e. RuleModule and friends)

Surely you would have been better off extending the ESTree types and being compatible with ESLint, being that you're implementing an ESLint plugin after all.

is there some reason you went down this path?

We are struggling to move our plugins over to typescript-eslint from the obsolete tslint project because of this.

The text was updated successfully, but these errors were encountered:

bradzacher · 2019-04-07T02:27:50Z

The answer is - because we have to.

For example, imagine that you have a base set of types as follows:

interface Bar {
  prop: string
}
interface Foo {
  prop: Bar
}

If we want to extend these to add additional fields, we can do that easily:

import { Foo } from 'base';

interface Foo {
  newProp: number
}

// valid TS
const x: Foo = {
  prop: {
    prop: '',
  },
  newProp: 1,
};

This is fine if you are solely adding new fields.
However, we have new node types - all of the TS* variants that we introduce as part of ts-estree.
A huge portion of the estree nodes are linked together. For example, this is a DoWhileStatement node:

interface DoWhileStatement {
  type: 'DoWhileStatement';
  test: Expression;
  body: Statement;
}

Note that the body and test properties link to intersection types which include the current ESTree node lists.
As part of our typescript enhanced estree definition, we have new Expression nodes to add in, so the question is - can we get those types into those intersection types to make our nodes valid based on the existing types?

We cannot extend the base types to add our own ones into the unions:

type Expression = Foo | Bar

// Duplicate identifier 'Expression'.
type Expression = Foo | Bar | Baz

We cannot override the properties on the estree node to make it consider our unions:

import { Node } from 'base';
interface Node {
  // Subsequent property declarations must have the same type.  Property 'body' must be of type 'Expression', but here has type 'Expression | Baz'.
  body: Expression | Baz
}

So there is no way to extend the base ESTree type definitions such that their intersections include our new typescript node types.

This means that we have to redefine every single ESTree node such that they include our special intersection types (i.e. https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/typescript-estree/src/ts-estree/ts-estree.ts)

This problem then bubbles up into the @types/eslint typings. They directly reference the base estree types, but we've had to redefine every single estree interface as our own. As we know we can't override the properties, we have to redefine the eslint types as well so that they point at our types.

As part of doing this, we looked at how we wanted contributors to write rules and what optional features we wanted people to use. Previously we were using an eslint plugin (eslint-plugin-eslint-plugin) to enforce this, but considering we now have complete control of the types, we instead enforced this at compile time by crafting the definitions such that they match exactly how we want our rules to look and what features we want people to use.

Your node types use an enum but really should use constant string types for consistency with ESTree

We use an enum because it gives very helpful typeahead/intellisense for consuming/comparing node types.
Additionally, it is essentially equivalent in most cases in the code, though we enforce via code reviews that people don't use string constants. Note there is a rule in the works to make this automatic via lint rules (#315). We'll implement this rule within this repo when it's finished.

repl

enum StrEnum {
  Value = "Value"
}

const isEqual = StrEnum.Value === 'Value'; // === true

... especially since some of your types even use string constants rather than the enum

If there are any nodes that use string constants, raise an issue, or a PR - and let us know. I'm pretty sure we had them all correct.
The string constants that exist in the ts-eslint.ts file are there intentionally AFAIK, I could be wrong though.

We can't really make plugins which use typescript-eslint nodes because your types are internal? (your cloned types, i.e. RuleModule and friends)

#330
People have asked. It's on the to do list.
I've started working on this issue specifically, however, because I think that it's an important thing to get done for a number of reasons (https://github.com/typescript-eslint/typescript-eslint/tree/utils-pkg).
Between a burst appendix, recovery, work, personal life, etc - I haven't had enough time to make a giant amount of progress on it. It's my priority for when I get back from my holiday though.

43081j · 2019-04-07T14:21:14Z

ESLint compatibility

The core problem seems to be that the ESTree types are not exactly extensible and were probably not designed with that in mind. This is why TypeScript uses keyof in its DOM types, for example, as it can then provide an interface you can extend.

Being that we have no such extensible type from ESTree, you have been forced into having to reimplement everything (a really not-so-nice thing to do, but there was no choice, I see).

We should consider contributing to the ESTree types so this can be fixed rather than doing such an unfortunate workaround.

Enums

My point was more that it is inconsistent with how ESTree types work. But being that the two can't be mixed anyway because of this implementation, I guess it isn't important and the ability to get good completions is nice.

Plugins

We're currently trying to move several plugins over to typescript-eslint, and this isn't possible yet due to the closed types. If you need any help just point at the issues and we can try contribute.

bradzacher · 2019-04-09T16:29:58Z

We should consider contributing to the ESTree types so this can be fixed rather than doing such an unfortunate workaround.

The problem is that this is not a case of poorly defined type defs in @types/estree, but a limitation of typescript itself.
AFAIK, there just isn't a way to override a property's on an existing type/interface.
That means that no matter how you define the base types, there's no way to add new AST node types without redefining absolutely everything.

Enums: My point was more that it is inconsistent with how ESTree types work

@types/estree's constant strings and our enum AST_NODE_TYPES should be equivalent and compatible both at compile-time and run-time:

enum Foo {
    Bar = 'Bar'
}

interface ESTree {
    type: 'Bar'
}
interface TSESTree {
    type: Foo
}

const x: TSESTree = {
    type: Foo.Bar,
};
const y: ESTree = x; // no error

repl

The thing is, however, that you'll get errors when assigning the real types together because of extra(/missing) properties, or because the intersection types don't match up.

The entire reason that we chose to use an enum is because it ensures that even if you somehow coerce the type of the property to string, you still get compile-time safety.

enum Foo {
  Bar = 'Bar'
}

interface TSESTree {
  type: Foo
}

const node: TSESTree = {
  type: Foo.Bar,
}

const nodeType: string = node.type;
if (nodeType === 'BarTypo') {
  // compile-time safe
}
if (nodeType === Foo.BarTypo) {
  // fails at compile time
}

repl

It also means you have concrete links to when the specific node type string is used via typescript's "Find Usages" feature (rather than doing a fuzzy search across the codebase for 'DoWhileStatement').

Plugins

There aren't that many eslint plugins that are built around the parser's nodes that are also written in typescript - it's a small fraction of a fraction of the user base.
In the first release of actually typing the plugin (#120) my focus was to just get the plugin typed and working so that everyone was on the same page - external use cases weren't on my mind at the time.

Having let the types sit for about 2 months now, the types themselves seem sound (nobody's been fighting or complaining about them within the plugin context that I've seen), so it's a good time to go back and clean them up.

I'm back from holidays now, so once I catch up on all of the PRs and issues from when I was away, I'll be working on the refactor to clean up and expose the types.

43081j · 2019-04-12T15:28:06Z

I'm currently having a stab at moving dtslint over to use this project so we can finally get it off tslint, so it would be great if we could get those duplicated types exposed. That way even if its a bit unfortunate they have to exist, at least they can be used.

About modifying the ESTree types... this is kinda why the core TS lib types use maps:

interface Foo {
  type: keyof FooTypeMap
}

interface FooTypeMap {
  'my-type': MyType
}

As you can augment/extend FooTypeMap by redefining it partially.

This only solves a couple of problems, though. It wouldn't directly help with the intersection issue (the fact that an Expression isn't extensible). It may not apply here so much as the types are little less flexible already and it might be an inappropriate solution for what we have to extend.

What you already have is fine, it would just be nice if there was a solution to allow the base types to be extended.

crhistianramirez · 2019-07-11T11:38:48Z

Looks like this issue should be closed? There's a merged PR from back in April

j-f1 · 2019-07-11T15:12:26Z

cc @bradzacher — GitHub will only auto-close the issues if you put fixes/closes before each reference:

# Works
fixes #123, fixes #456

# Doesn’t work
fixes #123, #456

bradzacher · 2019-07-11T15:15:58Z

Thanks! Yeah, I figured that out the hard way shortly after those PRs. Looks like these issues got missed in the manual cleanups james and I did after closing the PRs I'd done that on.

Kind of lame by github that they don't have support for it though!

43081j added package: typescript-estree Issues related to @typescript-eslint/typescript-estree triage Waiting for team members to take a look labels Apr 6, 2019

bradzacher added question Questions! (i.e. not a bug / enhancment / documentation) and removed triage Waiting for team members to take a look labels Apr 11, 2019

bradzacher mentioned this issue Apr 11, 2019

feat: Move shared types into their own package #425

Merged

bradzacher added the has pr there is a PR raised to close this label Apr 25, 2019

j-f1 closed this as completed Jul 11, 2019

github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

why are the ESTree types distinct from ESTree? #413

why are the ESTree types distinct from ESTree? #413

why are the ESTree types distinct from ESTree? #413

why are the ESTree types distinct from ESTree? #413

Comments

ESLint compatibility

Enums

Plugins