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(ast-spec): make BaseNode & BaseToken more type-safe #3560

Merged
merged 2 commits into from Jul 31, 2021

Conversation

MichaelDeBoey
Copy link
Contributor

@MichaelDeBoey MichaelDeBoey commented Jun 20, 2021

Make sure BaseNode's type is of type AST_NODE_TYPES and BaseToken's type is of type AST_TOKEN_TYPES

@typescript-eslint
Copy link
Contributor

typescript-eslint bot commented Jun 20, 2021

Thanks for the PR, @MichaelDeBoey!

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.

@MichaelDeBoey MichaelDeBoey force-pushed the patch-17 branch 4 times, most recently from acf0a25 to 4fc73f9 Compare Jun 20, 2021
Copy link
Member

@bradzacher bradzacher left a comment

thanks!

packages/ast-spec/src/base/BaseNodeOrToken.ts Outdated Show resolved Hide resolved
@bradzacher bradzacher added enhancement New feature or request awaiting response Issues waiting for a reply from the OP or another party labels Jun 20, 2021
@MichaelDeBoey MichaelDeBoey requested a review from bradzacher Jun 20, 2021
@@ -248,7 +248,9 @@ interface RuleContext<

// This isn't the correct signature, but it makes it easier to do custom unions within reusable listeners
// never will break someone's code unless they specifically type the function argument
type RuleFunction<T extends TSESTree.BaseNode = never> = (node: T) => void;
type RuleFunction<T extends TSESTree.Node | TSESTree.Token = never> = (
Copy link
Member

@bradzacher bradzacher Jun 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this before - it's not possible for you to create a selector on a token - so we don't need the TSESTree.Token type here.

Also it's been a while - but I think that when I defined this I didn't use the TSESTree.Node type for two reasons: (1) because it means if you've got some custom AST nodes from another parser - you can't use them, and (2) IIRC there was some perf issues with TS due to the massive size of the Node union.

Copy link
Contributor Author

@MichaelDeBoey MichaelDeBoey Jun 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll export TokenBase and do TSESTree.NodeBase | TSESTree.TokenBase instead

Copy link
Contributor Author

@MichaelDeBoey MichaelDeBoey Jun 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or should I do it with NodeAndTokenData instead?

Copy link
Member

@bradzacher bradzacher Jun 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need the token type at all because tokens aren't valid - so you can keep it as just BaseNode

Copy link
Contributor Author

@MichaelDeBoey MichaelDeBoey Jun 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need Token as we're also having a RuleListener for Comment & Token

Comment?: RuleFunction<TSESTree.Comment>;

Token?: RuleFunction<TSESTree.Token>;

Copy link
Member

@bradzacher bradzacher Jun 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird. I don't think either of those actually work.
Maybe they did at some point - but yeah a quick test doesn't look like they work any more.
Maybe - i only quickly tested against astexplorer's version of ESLint (v4).. so worth a quick test against ESLint v7 to confirm

Copy link
Contributor Author

@MichaelDeBoey MichaelDeBoey Jun 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I should delete these 2 then?

Copy link
Contributor Author

@MichaelDeBoey MichaelDeBoey Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented them out (for now) and nothing seems to break

Copy link
Contributor Author

@MichaelDeBoey MichaelDeBoey Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradzacher Should I just remove them?
Or what do you think is the best approach now?

@MichaelDeBoey MichaelDeBoey force-pushed the patch-17 branch 4 times, most recently from 398110a to 8933591 Compare Jun 21, 2021
@codecov
Copy link

codecov bot commented Jun 21, 2021

Codecov Report

Merging #3560 (77a0f00) into master (ce984e3) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3560   +/-   ##
=======================================
  Coverage   92.63%   92.64%           
=======================================
  Files         325      327    +2     
  Lines       11327    11334    +7     
  Branches     3195     3195           
=======================================
+ Hits        10493    10500    +7     
  Misses        370      370           
  Partials      464      464           
Flag Coverage Δ
unittest 92.64% <ø> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
packages/typescript-estree/src/convert.ts 98.27% <ø> (ø)
packages/visitor-keys/src/get-keys.ts 100.00% <0.00%> (ø)
packages/visitor-keys/src/visitor-keys.ts 100.00% <0.00%> (ø)

@MichaelDeBoey MichaelDeBoey requested a review from bradzacher Jun 21, 2021
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 28, 2021
Copy link
Member

@bradzacher bradzacher left a comment

lgtm - thanks for this!

@bradzacher bradzacher merged commit a6c5604 into typescript-eslint:master Jul 31, 2021
11 checks passed
@MichaelDeBoey MichaelDeBoey deleted the patch-17 branch Jul 31, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants