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.

Sign up for GitHub

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

Jump to bottom

feat(ast-spec): make BaseNode & BaseToken more type-safe #3560

Merged
merged 2 commits into from Jul 31, 2021
Merged

feat(ast-spec): make BaseNode & BaseToken more type-safe #3560

merged 2 commits into from Jul 31, 2021

Conversation

MichaelDeBoey
Copy link
Contributor

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

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.

@nx-cloud
Copy link

nx-cloud bot commented Jun 20, 2021 •
edited

Nx Cloud Report

CI ran the following commands for commit 77a0f00. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx test @typescript-eslint/eslint-plugin
#000000 nx test @typescript-eslint/eslint-plugin-internal
#000000 nx test @typescript-eslint/eslint-plugin-tslint
#000000 nx test @typescript-eslint/experimental-utils
#000000 nx test @typescript-eslint/parser
#000000 nx test @typescript-eslint/scope-manager
#000000 nx test @typescript-eslint/typescript-estree
#000000 nx test @typescript-eslint/visitor-keys

Sent with 💌 from NxCloud.

@MichaelDeBoey MichaelDeBoey force-pushed the patch-17 branch 4 times, most recently from acf0a25 to 4fc73f9 Compare June 20, 2021 16:55
bradzacher
bradzacher requested changes Jun 20, 2021
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

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
bradzacher
bradzacher reviewed Jun 20, 2021
View reviewed changes
packages/experimental-utils/src/ts-eslint/Rule.ts Outdated
@@ -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

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

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

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

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

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

typescript-eslint/packages/experimental-utils/src/ts-eslint/Rule.ts

Line 280 in abfc19b

Comment?: RuleFunction<TSESTree.Comment>;

typescript-eslint/packages/experimental-utils/src/ts-eslint/Rule.ts

Line 341 in abfc19b

Token?: RuleFunction<TSESTree.Token>;

Copy link
Member

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

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

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

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 June 21, 2021 09:02
@codecov
Copy link

codecov bot commented Jun 21, 2021 •
edited

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%> (ø)

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 28, 2021
bradzacher
bradzacher approved these changes Jul 31, 2021
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

lgtm - thanks for this!

@bradzacher bradzacher merged commit a6c5604 into typescript-eslint:master Jul 31, 2021
@MichaelDeBoey MichaelDeBoey deleted the patch-17 branch July 31, 2021 23:57
@MichaelDeBoey MichaelDeBoey mentioned this pull request Jul 31, 2021
Merged
@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.
Reviewers

@bradzacher bradzacher bradzacher approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@MichaelDeBoey @bradzacher