-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat: refactor to split AST specification out as its own module #2911
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
Conversation
Thanks for the PR, @bradzacher! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
2be09bf
to
f8b983b
Compare
f8b983b
to
dc7e3cb
Compare
ab8195c
to
56232c8
Compare
dc7e3cb
to
43d7bc4
Compare
43d7bc4
to
36c8a52
Compare
Codecov Report
@@ Coverage Diff @@
## master #2911 +/- ##
==========================================
- Coverage 92.86% 92.85% -0.01%
==========================================
Files 318 318
Lines 11048 11048
Branches 3129 3128 -1
==========================================
- Hits 10260 10259 -1
- Misses 350 352 +2
+ Partials 438 437 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
few notes:
we could actually follow a lead of typescript or estree on this eg, generally i like this idea, but i feel like this is a little to granular, maybe if we group them a little better we can have set of files that can contain multiple definition |
sounds like a good idea
This "grouping" was based on the union types we had declared in the old mega file. We can nest/organise however we like!
Could you provide an example of what you mean by this?
I wanted each node to have its own folder so that everything is isolated. Eventually I want to move all the fixtures into this folder as well, and orgnaise them appropriately (even duplicate them as necessary). The idea is that this "package" will document the types a node adheres to, and includes the examples that can generate each type. Ideally even going as far as attempting to do every combination for a child. It'll be verbose AF, but it ensures our spec is correct, and it provides clear documentation for consumers (and provides great examples for people to borrow and test against). |
there is one more thing that bothers me, new package, why do we need it, why not just merge it with types, as thats what this is, types |
For the moment, this is just types. But as mentioned - I want to move the fixtures here, and I want to add markdown docs to describe what a node is and what it represents. I plan on this being a private package, and using a build step to keep the published types in the |
moving fixtures is not really good idea, as we should not ask everyone who wants to use this package to include them in their build this package is included in types |
To clarify. This PR should result in no actual change in the external API(s).
|
07860e0
to
f69c8ef
Compare
@armano2 - if you wanted to take another eyeball - this is in a good state to merge as is. |
Fixes #2726 Fixes #2912 This PR is the basis for a big cleanup and reorganisation of the AST. This first step takes the file `types/src/ts-estree.ts` and splits it up in its entirety. This file was a monolith at 1700 lines - meaning it was a pain to organise and manage, and there was no way to isolate/restrict certain things (aside from adding comments). This PR should ultimately be a no-op - there should be no breaking changes here. I did fix up a number of types which I found when organising files into their folders. Whilst this PR ultimately creates more LOC, the isolation enables a few things: - By splitting the AST into its own module, it's isolated so easier to manage / govern - By splitting each AST node into its own folder we can cleanly document and link to individual node specs - By grouping nodes decls by folder, it's easier to inspect the types to validate unions are correct. - I found a number of invalid nodes in unions in this PR which have been fixed. - In a future PR we can: - Add lint rule(s) to validate unions are correct (eg ensure all `Expression` types are included in the `Expression` union). - Easily add documentation about the node without cluttering things up - Colocate fixtures/snapshots with the node specs to document the cases that we expect a node to show up - Colocate the conversion logic here so that it's easier to validate that the spec and the conversion logic are in sync - This will make it much easier to implement and maintain #1852
Reference issue: #2726
This PR is the basis for a big cleanup and reorganisation of the AST.
This first step takes the file
types/src/ts-estree.ts
and splits it up in its entirety.This file was a monolith at 1700 lines - meaning it was a pain to organise and manage, and there was no way to isolate/restrict certain things (aside from adding comments).
This PR should ultimately be a no-op - there should be no breaking changes here.
I did fix up a number of types which I found when organising files into their folders.
Whilst this PR ultimately creates more LOC, the isolation enables a few things:
Expression
types are included in theExpression
union).TODO:
Nodes
union/types