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

Unify assertions AST with Babel #1158

Closed
thorn0 opened this issue Oct 29, 2019 · 8 comments
Closed

Unify assertions AST with Babel #1158

thorn0 opened this issue Oct 29, 2019 · 8 comments
Labels
awaiting response Issues waiting for a reply from the OP or another party external This issue is with another package, not typescript-eslint itself package: typescript-estree Issues related to @typescript-eslint/typescript-estree

Comments

@thorn0
Copy link
Contributor

thorn0 commented Oct 29, 2019

The Babel team named the node property assertsModifier, not asserts. Let's rename it while it's not too late, shall we?

babel/babel#10543

@thorn0 thorn0 added package: typescript-estree Issues related to @typescript-eslint/typescript-estree triage Waiting for maintainers to take a look labels Oct 29, 2019
@bradzacher
Copy link
Member

bradzacher commented Oct 29, 2019

Shouldn't it be the other way round?
We've had this merged and released for 2 weeks (2 minor releases), so it is well and truly out there. Changing it would require a breaking change.

Babel merged theirs in 44 minutes ago, and I don't think they've released it - so it'd be better for them to align to us.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party external This issue is with another package, not typescript-eslint itself and removed triage Waiting for maintainers to take a look labels Oct 29, 2019
@thorn0
Copy link
Contributor Author

thorn0 commented Oct 29, 2019

Shouldn't it be the other way round?

Probably. I read this in readme

Some of the people involved in typescript-eslint are also involved in Babel and babel-eslint, and in this project we are working hard to align on the AST format for non-standard JavaScript syntax. This is an ongoing effort.

but didn't know how exactly this process works and which project is considered the source of truth.

@bradzacher
Copy link
Member

Looking at that PR, nobody involved in this project was pulled in at all to eyeball, and none of the reviewers are involved here as well.

James has been involved in the babel side of things in the past but he's been pretty busy with real life stuff. We have a few other people who are collaborators on both projects, but then that relies them seeing the work in both repos (or being notified).

@existentialism
Copy link
Member

existentialism commented Oct 29, 2019

@bradzacher sorry I missed this (been traveling quite a bit). re: alignment, feel free to ping me as things get ready to land here for TS (might even be nice to setup a group to cc with @hzoo and @nicolo-ribaudo too), and we'll be sure /cc you and James on the Babel-side!

@thorn0
Copy link
Contributor Author

thorn0 commented Oct 29, 2019

@bradzacher And what is the process supposed to be for the existing differences? E.g. for TSImportType, Babel uses the argument property name, typescript-eslint uses parameter. How are these things going to be aligned?

@bradzacher
Copy link
Member

re: alignment

Definitely, I think it makes sense to add a collaborator list here so we can easily tag the relevant babel peeps when we are making / talking about the AST (and vice versa for babel when the TS AST is being worked on).

And what is the process supposed to be for the existing differences

We don't have one!
We need to "sit down and talk" about these differences so that we can figure out what breaking changes are required where so that we can converge the two representations.

There's a bunch of stuff that I've been thinking about around this, because there's a lot of weirdness in the AST.

@nicolo-ribaudo
Copy link

We should definitely ping each other when designing new ASTs. We already have a #typescript room in our Slack channel, we might use it for any chat which doesn't fit well the GitHub issue format.

Regarding the AST differences, do you already know the complete set of them?

@bradzacher
Copy link
Member

bradzacher commented Oct 29, 2019

We have a basic alignment pipeline setup in this repo, but since james set it up, it has fallen a bit into disrepair.
Between not keeping it up to date with the latest version of babel, our teams not fully aligning on things (like babel having some props being optional when we always emit them), and some things differing, it's not showing a great state.

Here is a list of our fixtures where the two impls differ in some way.
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/typescript-estree/tests/ast-alignment/fixtures-to-test.ts

This test can definitely be improved (like ignoring times that we emit prop: false and babel emitting no prop), but it's at least a start.

Side note that we probably need to have a lot more cases added to our fixtures, I know there's a number of edge case gaps that we don't have proper testing for (eg #1100).


We already have a #typescript room in our Slack channel, we might use it for any chat which doesn't fit well the GitHub issue format.

I just joined the slack + channel

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting response Issues waiting for a reply from the OP or another party external This issue is with another package, not typescript-eslint itself package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
Development

No branches or pull requests

4 participants