feat(experimental-utils): extract `isNodeOfTypeWithConditions` out of `ast-utils`' `predicates` #3837

MichaelDeBoey · 2021-09-04T09:27:18Z

Like #3677, but with extra conditions

typescript-eslint · 2021-09-04T09:27:21Z

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 · 2021-09-04T09:35:18Z

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

nx-cloud · 2021-09-04T09:35:19Z

Nx Cloud Report

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

📂 See all runs for this branch

Status	Command
	`nx test @typescript-eslint/eslint-plugin-internal`
	`nx test @typescript-eslint/eslint-plugin-tslint`
	`nx test @typescript-eslint/eslint-plugin`
	`nx test @typescript-eslint/eslint-plugin-tslint`
	`nx test @typescript-eslint/eslint-plugin`
	`nx test @typescript-eslint/eslint-plugin-tslint`
	`nx test @typescript-eslint/eslint-plugin`
	`nx run-many --target=typecheck --all --parallel`
	`nx run-many --target=build --all --parallel`
	`nx test @typescript-eslint/parser`
	`nx test @typescript-eslint/parser`
	`nx test @typescript-eslint/experimental-utils`
	`nx test @typescript-eslint/experimental-utils`
	`nx test @typescript-eslint/scope-manager`
	`nx test @typescript-eslint/visitor-keys`
	`nx test @typescript-eslint/typescript-estree`
	`nx run-many --target=build --all --parallel`
	`nx run-many --target=build --all --parallel`
	`nx test @typescript-eslint/scope-manager`
	`nx test @typescript-eslint/visitor-keys`

Sent with 💌 from NxCloud.

bradzacher

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.

MichaelDeBoey · 2021-09-20T22:48:40Z

@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 · 2021-09-21T00:06:21Z

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 · 2021-09-21T09:58:09Z

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

@bradzacher What do mean exactly by this?

packages/experimental-utils/src/ast-utils/predicates.ts

… `ast-utils`' `predicates`

MichaelDeBoey · 2021-09-22T10:27:24Z

packages/experimental-utils/src/ast-utils/predicates.ts

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


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 🤷‍♂️

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 · 2021-10-03T21:32:22Z

thanks for this!

MichaelDeBoey force-pushed the patch-27 branch 2 times, most recently from 83ae194 to d5e8fd7 Compare September 8, 2021 08:28

bradzacher requested changes Sep 20, 2021

View reviewed changes

bradzacher added awaiting response Issues waiting for a reply from the OP or another party refactor PRs that refactor code only labels Sep 20, 2021

MichaelDeBoey requested a review from bradzacher September 20, 2021 22:48

MichaelDeBoey force-pushed the patch-27 branch from d5e8fd7 to 0b29c93 Compare September 20, 2021 22:54

bradzacher reviewed Sep 21, 2021

View reviewed changes

packages/experimental-utils/src/ast-utils/predicates.ts Outdated Show resolved Hide resolved

MichaelDeBoey force-pushed the patch-27 branch from 0b29c93 to 76a82cc Compare September 21, 2021 16:23

MichaelDeBoey requested a review from bradzacher September 22, 2021 10:08

MichaelDeBoey force-pushed the patch-27 branch from 76a82cc to 69f47c5 Compare September 22, 2021 10:09

feat(experimental-utils): extract isNodeOfTypeWithConditions out of…

ebf6f45

… `ast-utils`' `predicates`

MichaelDeBoey force-pushed the patch-27 branch from 69f47c5 to ebf6f45 Compare September 22, 2021 10:26

MichaelDeBoey commented Sep 22, 2021

View reviewed changes

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Oct 3, 2021

bradzacher approved these changes Oct 3, 2021

View reviewed changes

bradzacher merged commit 214f898 into typescript-eslint:master Oct 3, 2021

MichaelDeBoey deleted the patch-27 branch October 4, 2021 15:17

This was referenced Nov 1, 2021

build(deps-dev): bump @typescript-eslint/parser from 4.30.0 to 4.33.0 numbersprotocol/capture-lite#1025

Closed

build(deps-dev): bump @typescript-eslint/eslint-plugin from 4.30.0 to 4.33.0 numbersprotocol/capture-lite#1023

Closed

github-actions bot locked as resolved and limited conversation to collaborators Nov 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(experimental-utils): extract `isNodeOfTypeWithConditions` out of `ast-utils`' `predicates` #3837

feat(experimental-utils): extract `isNodeOfTypeWithConditions` out of `ast-utils`' `predicates` #3837

MichaelDeBoey commented Sep 4, 2021

typescript-eslint bot commented Sep 4, 2021

codecov bot commented Sep 4, 2021 •

edited

Loading

nx-cloud bot commented Sep 4, 2021 •

edited

Loading

bradzacher left a comment

MichaelDeBoey commented Sep 20, 2021 •

edited

Loading

bradzacher commented Sep 21, 2021

MichaelDeBoey commented Sep 21, 2021

MichaelDeBoey Sep 22, 2021 •

edited

Loading

bradzacher Oct 3, 2021

bradzacher commented Oct 3, 2021

feat(experimental-utils): extract isNodeOfTypeWithConditions out of ast-utils' predicates #3837

feat(experimental-utils): extract isNodeOfTypeWithConditions out of ast-utils' predicates #3837

Conversation

MichaelDeBoey commented Sep 4, 2021

typescript-eslint bot commented Sep 4, 2021

codecov bot commented Sep 4, 2021 • edited Loading

Codecov Report

nx-cloud bot commented Sep 4, 2021 • edited Loading

Nx Cloud Report

bradzacher left a comment

Choose a reason for hiding this comment

MichaelDeBoey commented Sep 20, 2021 • edited Loading

bradzacher commented Sep 21, 2021

MichaelDeBoey commented Sep 21, 2021

MichaelDeBoey Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

bradzacher Oct 3, 2021

Choose a reason for hiding this comment

bradzacher commented Oct 3, 2021

feat(experimental-utils): extract `isNodeOfTypeWithConditions` out of `ast-utils`' `predicates` #3837

feat(experimental-utils): extract `isNodeOfTypeWithConditions` out of `ast-utils`' `predicates` #3837

codecov bot commented Sep 4, 2021 •

edited

Loading

nx-cloud bot commented Sep 4, 2021 •

edited

Loading

MichaelDeBoey commented Sep 20, 2021 •

edited

Loading

MichaelDeBoey Sep 22, 2021 •

edited

Loading