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

Update: requireStringLiterals option for valid-typeof (fixes #6698) #6923

Merged
merged 1 commit into from
Aug 21, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions docs/rules/valid-typeof.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ For a vast majority of use cases, the result of the `typeof` operator is one of

This rule enforces comparing `typeof` expressions to valid string literals.

## Options

This rule has an object option:

* `"requireStringLiterals": true` requires `typeof` expressions to only be compared to string literals, and disallows comparisons to any other value.

Examples of **incorrect** code for this rule:

```js
Expand All @@ -28,6 +34,26 @@ typeof foo === baz
typeof bar === typeof qux
```

Examples of **incorrect** code with the `{ "requireStringLiterals": true }` option:

```js
typeof foo === undefined
typeof bar == Object
typeof baz === "strnig"
typeof qux === "some invalid type"
typeof baz === anotherVariable
typeof foo == 5
typeof bar === typeof qux
```

Examples of **correct** code with the `{ "requireStringLiterals": true }` option:

```js
typeof foo === "undefined"
typeof bar == "object"
typeof baz === "string"
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we want to mention something like typeof thing = "foo"? To clarify that we're not checking if it's a valid string literal.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, I guess we are checking that. In that case I would love the docs to mention this and the example I proposed should be listed in incorrect code.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's typeof baz === "strnig", which certainly signifies the intent (to catch typos), but it's easy to overlook. I'm +1 for adding typeof baz = "foo" or typeof baz = "someInvalidType"as another example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a couple more "invalid" examples.

```

## When Not To Use It

You may want to turn this rule off if you will be using the `typeof` operator on host objects.
20 changes: 17 additions & 3 deletions lib/rules/valid-typeof.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,17 @@ module.exports = {
recommended: true
},

schema: []
schema: [
{
type: "object",
properties: {
requireStringLiterals: {
type: "boolean"
}
},
additionalProperties: false
}
]
},

create(context) {
Expand All @@ -37,8 +47,12 @@ module.exports = {
if (parent.type === "BinaryExpression" && OPERATORS.indexOf(parent.operator) !== -1) {
const sibling = parent.left === node ? parent.right : parent.left;

if (sibling.type === "Literal" && VALID_TYPES.indexOf(sibling.value) === -1) {
context.report(sibling, "Invalid typeof comparison value.");
if (sibling.type === "Literal") {
if (VALID_TYPES.indexOf(sibling.value) === -1) {
context.report(sibling, "Invalid typeof comparison value.");
}
} else if (context.options[0] && context.options[0].requireStringLiterals) {
context.report(sibling, "Typeof comparisons should be to string literals.");
}
}
}
Expand Down
39 changes: 38 additions & 1 deletion tests/lib/rules/valid-typeof.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,19 @@ ruleTester.run("valid-typeof", rule, {
"typeof(foo) !== 'string'",
"typeof(foo) == 'string'",
"typeof(foo) != 'string'",
"var oddUse = typeof foo + 'thing'"
"var oddUse = typeof foo + 'thing'",
{
code: "typeof foo === 'number'",
options: [{ requireStringLiterals: true }],
},
{
code: "typeof foo === \"number\"",
options: [{ requireStringLiterals: true }]
},
{
code: "var baz = typeof foo + 'thing'",
options: [{ requireStringLiterals: true }]
}
],

invalid: [
Expand Down Expand Up @@ -94,6 +106,31 @@ ruleTester.run("valid-typeof", rule, {
{
code: "if (typeof bar == 'umdefined') {}",
errors: [{ message: "Invalid typeof comparison value.", type: "Literal" }]
},
{
code: "typeof foo == 'invalid string'",
options: [{ requireStringLiterals: true }],
errors: [{ message: "Invalid typeof comparison value.", type: "Literal" }]
},
{
code: "typeof foo == Object",
options: [{ requireStringLiterals: true }],
errors: [{ message: "Typeof comparisons should be to string literals.", type: "Identifier" }]
},
{
code: "typeof foo === undefined",
options: [{ requireStringLiterals: true }],
errors: [{ message: "Typeof comparisons should be to string literals.", type: "Identifier" }]
},
{
code: "undefined === typeof foo",
options: [{ requireStringLiterals: true }],
errors: [{ message: "Typeof comparisons should be to string literals.", type: "Identifier" }]
},
{
code: "undefined == typeof foo",
options: [{ requireStringLiterals: true }],
errors: [{ message: "Typeof comparisons should be to string literals.", type: "Identifier" }]
}
]
});