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

feat: refactor to split AST specification out as its own module #2911

Merged
merged 1 commit into from May 4, 2021

Conversation

bradzacher
Copy link
Member

@bradzacher bradzacher commented Jan 2, 2021

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:

  • 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

TODO:

  • separate all types from the monolithic file
  • add a test to ensure that all nodes are included in the Nodes union
  • add a build step to copy a bundled declaration file to /types
  • add a build doc generation step to allow for nice documentation on website
  • add fixtures for every single node type
    • Will do this separately - #TODO

@bradzacher bradzacher added the refactor PRs that refactor code only label Jan 2, 2021
@typescript-eslint
Copy link
Contributor

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.

@bradzacher bradzacher added the DO NOT MERGE PRs which should not be merged yet label Jan 2, 2021
@bradzacher bradzacher force-pushed the separate-spec branch 4 times, most recently from 2be09bf to f8b983b Compare January 2, 2021 21:26
@bradzacher bradzacher changed the base branch from master to union-intersection-sorting January 3, 2021 20:34
Base automatically changed from union-intersection-sorting to master January 4, 2021 01:12
@codecov
Copy link

codecov bot commented Jan 4, 2021

Codecov Report

Merging #2911 (a5f8933) into master (209f6d0) will decrease coverage by 0.00%.
The diff coverage is 44.44%.

@@            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     
Flag Coverage Δ
unittest 92.85% <44.44%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...lugin-internal/src/rules/plugin-test-formatting.ts 81.75% <ø> (ø)
...es/eslint-plugin/src/rules/no-loss-of-precision.ts 91.66% <ø> (ø)
...slint-plugin/src/rules/no-unnecessary-condition.ts 97.17% <ø> (ø)
...ages/eslint-plugin/src/rules/prefer-regexp-exec.ts 100.00% <ø> (ø)
.../src/rules/sort-type-union-intersection-members.ts 92.53% <ø> (ø)
packages/scope-manager/src/scope/ScopeBase.ts 91.12% <37.50%> (-0.60%) ⬇️
packages/typescript-estree/src/convert.ts 98.24% <100.00%> (ø)

@armano2
Copy link
Member

armano2 commented Feb 21, 2021

few notes:

  1. jsx should be in its own group, and not a "special"
  2. there should be separate group for Declarations and Statements -> Declarations are Statements

we could actually follow a lead of typescript or estree on this eg,
https://github.com/estree/estree/blob/390a050fd2d6a7b69688f1434e0929d7c7455d05/es5.md#statements

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

@bradzacher
Copy link
Member Author

jsx should be in its own group, and not a "special"

sounds like a good idea

there should be separate group for Declarations and Statements -> Declarations are Statements
we could actually follow a lead of typescript or estree on this

This "grouping" was based on the union types we had declared in the old mega file.
We didn't have a "declaration" union type - only DeclarationStatement (which didn't include variable declarations, hence I marked it for removal - it felt like a useless union)

We can nest/organise however we like!

maybe if we group them a little better we can have set of files that can contain multiple definition

Could you provide an example of what you mean by this?

but i feel like this is a little to granular

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.
EG if a property on a node has type "Expression", we should have one fixture which validates each expression type is valid.

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).

@armano2
Copy link
Member

armano2 commented Feb 21, 2021

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

@bradzacher
Copy link
Member Author

bradzacher commented Feb 21, 2021

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 types package.

@armano2
Copy link
Member

armano2 commented Feb 21, 2021

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

@bradzacher
Copy link
Member Author

bradzacher commented Feb 21, 2021

To clarify.

This PR should result in no actual change in the external API(s).

ast-spec would be a private package that is not published to NPM.
It will contain the following:

  • types for the AST (this PR)
  • markdown docs explaining the AST
  • fixtures

types will contain the types.
via a build step, we will bundle the types from ast-spec and copy them into the types package.
This way the fixtures, documentation, and folder structure are kept private and not published, and that way we don't need to publish another package for no reason.

@bradzacher bradzacher force-pushed the separate-spec branch 6 times, most recently from 07860e0 to f69c8ef Compare March 31, 2021 20:03
@bradzacher bradzacher removed the DO NOT MERGE PRs which should not be merged yet label Mar 31, 2021
@bradzacher bradzacher marked this pull request as ready for review March 31, 2021 22:17
@bradzacher
Copy link
Member Author

@armano2 - if you wanted to take another eyeball - this is in a good state to merge as is.
We can follow up with adding docs and fixtures in future PRs.

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
@bradzacher bradzacher merged commit 25ea953 into master May 4, 2021
@bradzacher bradzacher deleted the separate-spec branch May 4, 2021 23:15
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
refactor PRs that refactor code only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants