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(experimental-utils): extract isNodeOfTypeWithConditions out of ast-utils' predicates #3837

Merged
merged 1 commit into from Oct 3, 2021

Conversation

MichaelDeBoey
Copy link
Contributor

@MichaelDeBoey MichaelDeBoey commented Sep 4, 2021

Like #3677, but with extra conditions

@typescript-eslint
Copy link
Contributor

@typescript-eslint typescript-eslint bot commented Sep 4, 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.

@codecov
Copy link

@codecov codecov bot commented Sep 4, 2021

Codecov Report

Merging #3837 (ebf6f45) into master (dda9cee) will decrease coverage by 0.86%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master    #3837      +/-   ##
==========================================
- Coverage   93.51%   92.65%   -0.87%     
==========================================
  Files         151      192      +41     
  Lines        8144     8767     +623     
  Branches     2584     2694     +110     
==========================================
+ Hits         7616     8123     +507     
- Misses        167      260      +93     
- Partials      361      384      +23     
Flag Coverage Δ
unittest 92.65% <92.30%> (-0.87%) ⬇️

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

Impacted Files Coverage Δ
...gin-internal/src/rules/no-poorly-typed-ts-props.ts 88.88% <ø> (ø)
...ges/eslint-plugin/src/rules/no-inferrable-types.ts 93.33% <ø> (ø)
...nt-plugin/src/rules/no-unused-vars-experimental.ts 91.39% <ø> (ø)
...ges/experimental-utils/src/ast-utils/predicates.ts 54.83% <75.00%> (ø)
...plugin-internal/src/rules/prefer-ast-types-enum.ts 91.30% <100.00%> (ø)
...plugin/src/rules/explicit-module-boundary-types.ts 89.47% <100.00%> (ø)
packages/eslint-plugin/src/rules/no-shadow.ts 77.34% <100.00%> (+0.17%) ⬆️
packages/eslint-plugin/src/rules/no-type-alias.ts 100.00% <100.00%> (ø)
...es/eslint-plugin/src/rules/no-use-before-define.ts 91.20% <100.00%> (+0.09%) ⬆️
.../eslint-plugin/src/util/explicitReturnTypeUtils.ts 100.00% <100.00%> (ø)
... and 41 more

@MichaelDeBoey MichaelDeBoey force-pushed the patch-27 branch 2 times, most recently from 83ae194 to d5e8fd7 Compare Sep 8, 2021
Copy link
Member

@bradzacher bradzacher left a comment

I don't think that this a "good" change.
#3677 was great because it deduplicated a bunch of types AND it also kept the same perf with the constant logical expressions.

However this change adds in several function calls (Object.entries and Array.every) where before there was just one logical expression - which results in code that is 98% slower - https://jsbench.me/acktt3qk8u/1.

@bradzacher bradzacher added awaiting response refactor labels Sep 20, 2021
@MichaelDeBoey
Copy link
Contributor Author

@MichaelDeBoey MichaelDeBoey commented Sep 20, 2021

@bradzacher We're really talking about fractions over here.

The big benefit is that you don't have to declare the same thing in both return type and check, which for me is a big win in DX.

The Object.entries & Array.every are because the passed object could have multiple properties to be checked at (although that's not used currently).
I'm happy to implement it in another way if you see fit.

@bradzacher
Copy link
Member

@bradzacher bradzacher commented Sep 21, 2021

We're really talking about fractions over here.

In isolation - yes, but these functions can be called thousands of times each over a codebase - which can add up very quickly.
I guess though it doesn't matter toooo much as it's so small of a function.
These functions also aren't on any hot paths unless the code needs to be fixed - so they aren't called much.

Let's move the Object.entries call outside the inner function and that'll do.

@MichaelDeBoey
Copy link
Contributor Author

@MichaelDeBoey MichaelDeBoey commented Sep 21, 2021

Let's move the Object.entries call outside the inner function and that'll do.

@bradzacher What do mean exactly by this?

): ((
node: TSESTree.Node | null | undefined,
) => node is TSESTree.Node & { type: NodeType } & Conditions) => {
Copy link
Contributor Author

@MichaelDeBoey MichaelDeBoey Sep 22, 2021

Choose a reason for hiding this comment

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

Due to the extraction of entries & @typescript-eslint/explicit-function-return-type, we have to duplicate this return type

If you want, I can extract this into a NodeIsOfTypeWithConditionsFunction type if you want, but this won't be much clearer I'm afraid 🤷‍♂️

Copy link
Member

@bradzacher bradzacher Oct 3, 2021

Choose a reason for hiding this comment

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

I'm fine with the duplication on the util.
I'm also happy to just disable the linter on this check too.
We can merge this as is.

@bradzacher bradzacher removed the awaiting response label Oct 3, 2021
@bradzacher
Copy link
Member

@bradzacher bradzacher commented Oct 3, 2021

thanks for this!

@bradzacher bradzacher merged commit 214f898 into typescript-eslint:master Oct 3, 2021
11 checks passed
@MichaelDeBoey MichaelDeBoey deleted the patch-27 branch Oct 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants