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

[refactor] namespace parent member expression validation into separate helper function #2584

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

azyzz228
Copy link
Contributor

@azyzz228 azyzz228 commented Nov 1, 2022

Two slowest rules namespace and no-deprecated used almost same lines of code for validating member expressions. I refactored validation function into separate helper function namespaceValidator. Then, I changed namespace and no-deprecated to use namespaceValidator function.

@azyzz228 azyzz228 changed the title Refactor namespace parent member expression validation [refactor] namespace parent member expression validation into separate helper function Nov 1, 2022
package.json Outdated Show resolved Hide resolved
src/core/namespaceValidator.js Outdated Show resolved Hide resolved
src/core/namespaceValidator.js Outdated Show resolved Hide resolved
src/core/namespaceValidator.js Outdated Show resolved Hide resolved
src/rules/namespace.js Outdated Show resolved Hide resolved
Comment on lines 11 to 19
if (onComputed(node) === 'return') {
return;
}
}

if (onNamespaceNotFound) {
if (onNamespaceNotFound(node, namespace, namepath) === 'return') {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

hm, do we need these early returns?

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 re-produced exact flow as in namespace rules: https://github.com/import-js/eslint-plugin-import/blob/main/src/rules/namespace.js. Lines 124-140.

Line 139 of original namespace rule has break instead of return, but because nothing comes after while loop, break does the same thing as return?

Copy link
Member

Choose a reason for hiding this comment

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

Are there any cases of calling onNamespaceNotFound where we don't want to break/return?

Copy link
Contributor Author

@azyzz228 azyzz228 Nov 1, 2022

Choose a reason for hiding this comment

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

Yes. onNamespaceNotFound is only used in namespace rule.

If we can find node's name in namespace map, then but just proceed execution. We only return/break when we cannot find node's name in namespace map.

Below is onNamespaceNotFound in namespace rule, we pass what's below to our helper function.

        function onNamespaceNotFound(context) {
          return function inner(node, namespace, namepath) {
            if (!namespace.has(node.property.name)) {
              context.report(
                node.property,
                makeMessage(node.property, namepath),
              );
              return 'return';
            }
          };
        }

Copy link
Member

Choose a reason for hiding this comment

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

gotcha, hmm.

not a fan of returning the string "return", maybe it could be a true or something, but returning a sentinel value seems to be the only solution.

ahhhh wait! what if the inner callback took a fourth argument, that meant "return", for it to return? that way "what kind of value if is" is up to the helper implementation

Copy link
Contributor Author

@azyzz228 azyzz228 Nov 2, 2022

Choose a reason for hiding this comment

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

That's brilliant! I implemented it here: azyzz228@5985656. Because no-deprecated rule for now just skips computed, meanwhile namespace one works through it and takes in node as an argument, I used arguments[2] in no-deprecated's onComputed to communicate return to namespaceValidator

src/rules/namespace.js Outdated Show resolved Hide resolved
src/rules/no-deprecated.js Outdated Show resolved Hide resolved
src/rules/no-deprecated.js Outdated Show resolved Hide resolved
@azyzz228 azyzz228 force-pushed the refactor-namespace-parent-member-expression-validation branch from e305ed0 to 5985656 Compare November 2, 2022 03:12
@azyzz228 azyzz228 marked this pull request as ready for review November 14, 2022 18:16
@ljharb ljharb force-pushed the refactor-namespace-parent-member-expression-validation branch from 5985656 to 516b80a Compare January 11, 2023 23:05
@ljharb
Copy link
Member

ljharb commented Aug 30, 2024

@azyzz228 this PR will need a rebase, and unfortunately it doesn't seem trivial.

@ljharb ljharb marked this pull request as draft August 30, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants