Fix: dot notation rule failing to catch string template (fixes #9350)

[philquinn]

Fixes #9350

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[X ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

I added a check for TemplateLiteral and if it is the case then it checks the quasi associated as long as no expression is found, as if it has an expression is not a valid dot notation.

As the fix would be reused in my chance I pulled that into a function.

Added a few test cases.

Is there anything you'd like reviewers to focus on?

Nothing in particular

[eslintbot]

Thanks for the pull request, @philquinn! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[jsf-clabot]

All committers have signed the CLA.

[eslintbot]

Thanks for the pull request, @philquinn! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[eslintbot]

LGTM

[not-an-aardvark]

Thanks for the pull request! This looks pretty good overall. I left a few small suggestions.

[not-an-aardvark]

I think checking node.property.quasis.length is redundant here, because that will always be the case when node.property.expressions.length === 0.

[philquinn]

Really fair, I was being a bit defensive with checking here

[not-an-aardvark]

I think calling String() is unnecessary here, because the value of the template literal will always be a string.

[not-an-aardvark]

In general I think it would be better to use the .cooked property of the template literal in these cases, rather than the .raw property. The difference is that if a template literal contains escape sequences like \n, the .cooked property will contain the "computed" escape sequence (in this case, a newline) whereas the .raw property will contain the raw escape sequence that was present in the string (in this case, a backslash followed by an n).

Using cooked here would ensure that the rule reports an error when linting strange code like foo[`b\ar`], which is equivalent to foo.bar.

[not-an-aardvark]

It seems like there's some duplicated code here relating to checking keywords, checking valid identifiers, and reporting. I think that could be avoided by doing something like this:

function checkComputedProperty(node, value) {
    if (validIdentifier.test(value) && /* ... */ ) {
        // ...
    }
}

return {
    MemberExpression(node) {
        if (node.computed && node.property.type === "Literal") {
            checkComputedProperty(node, node.property.value);
        } else if (node.computed && node.property.type === "TemplateLiteral" && node.property.expressions.length === 0) {
            checkComputedProperty(node, node.property.quasis[0].value.cooked);
        }

    }
};

This would also make fixDotNotationWithValue unnecessary, since there would only be one context.report call in the file. In other words, this would extract all of the value-checking logic into a function rather than only extracting the autofix logic.

[philquinn]

Yeah agree thats cleaner. Will change now.

[eslintbot]

LGTM

[philquinn]

@not-an-aardvark Updated PR there. Thanks for the review.

[not-an-aardvark]

Looks good to me. Thanks!

[platinumazure]

LGTM, thanks for contributing!

[philquinn]

Thanks for the reviews

[kaicataldo]

LGTM. Thanks for contributing to ESLint!