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

Add useAt option to autofix .lastObject to .at(-1) in no-get rule #1847

Merged
merged 3 commits into from
May 31, 2023
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions docs/rules/no-get.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ import ObjectProxy from '@ember/object/proxy';
export default ObjectProxy.extend({
someFunction() {
const foo = this.get('propertyInsideProxyObject'); // Allowed because inside proxy object.
}
},
});
```

Expand All @@ -85,7 +85,7 @@ export default EmberObject.extend({
unknownProperty(key) {},
someFunction() {
const foo = this.get('property'); // Allowed because inside object implementing `unknownProperty()`.
}
},
});
```

Expand All @@ -95,6 +95,7 @@ This rule takes an optional object containing:

- `boolean` -- `ignoreGetProperties` -- whether the rule should ignore `getProperties` (default `false`)
- `boolean` -- `ignoreNestedPaths` -- whether the rule should ignore `this.get('some.nested.property')` (can't be enabled at the same time as `useOptionalChaining`) (default `false`)
- `boolean` -- `useAt` -- whether the rule should use `at(-1)` [Array.prototype.at()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/at) to replace `lastObject` (default `false`) (TODO: enable by default once we require Node 16.6)
- `boolean` -- `useOptionalChaining` -- whether the rule should use the [optional chaining operator](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining) `?.` to autofix nested paths such as `this.get('some.nested.property')` to `this.some?.nested?.property` (when this option is off, these nested paths won't be autofixed at all) (default `true`)
- `boolean` -- `catchSafeObjects` -- whether the rule should catch non-`this` imported usages like `get(foo, 'bar')` (default `true`)
- `boolean` -- `catchUnsafeObjects` -- whether the rule should catch non-`this` usages like `foo.get('bar')` even though we don't know for sure if `foo` is an Ember object (default `false`)
Expand Down
56 changes: 38 additions & 18 deletions lib/rules/no-get.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function isValidJSPath(str) {
return str.split('.').every((part) => isValidJSVariableName(part) || isValidJSArrayIndex(part));
}

function reportGet({ node, context, path, useOptionalChaining, objectText, sourceCode }) {
function reportGet({ node, context, path, useAt, useOptionalChaining, objectText, sourceCode }) {
const isInLeftSideOfAssignmentExpression = utils.isInLeftSideOfAssignmentExpression(node);
context.report({
node,
Expand All @@ -34,6 +34,7 @@ function reportGet({ node, context, path, useOptionalChaining, objectText, sourc
fixer,
path,
useOptionalChaining,
useAt,
isInLeftSideOfAssignmentExpression,
objectText,
sourceCode,
Expand All @@ -47,6 +48,7 @@ function fixGet({
fixer,
path,
useOptionalChaining,
useAt,
isInLeftSideOfAssignmentExpression,
objectText,
sourceCode,
Expand All @@ -63,12 +65,14 @@ function fixGet({
if (types.isConditionalExpression(path)) {
const newConsequentExpression = convertLiteralTypePath({
path: path.consequent.value,
useAt,
useOptionalChaining,
shouldIgnoreOptionalChaining,
objectText,
});
const newAlternateExpression = convertLiteralTypePath({
path: path.alternate.value,
useAt,
useOptionalChaining,
shouldIgnoreOptionalChaining,
objectText,
Expand All @@ -92,6 +96,7 @@ function fixGet({

const replacementPath = convertLiteralTypePath({
path,
useAt,
useOptionalChaining,
shouldIgnoreOptionalChaining,
objectText,
Expand Down Expand Up @@ -137,7 +142,7 @@ function fixGetProperties({ fixer, node, objectText, propertyNames }) {
// When destructuring assignment is in the left side of "=".
// Example: const { foo, bar } = getProperties(this.obj, "foo", "bar");
// Expectation:
// const const { foo, bar } = this.obj;
// const { foo, bar } = this.obj;

return fixer.replaceText(node, `${objectText}`);
}
Expand Down Expand Up @@ -183,6 +188,10 @@ module.exports = {
type: 'boolean',
default: false,
},
useAt: {
type: 'boolean',
default: false,
},
},
additionalProperties: false,
},
Expand All @@ -192,6 +201,7 @@ module.exports = {
// Options:
const ignoreGetProperties = context.options[0] && context.options[0].ignoreGetProperties;
const ignoreNestedPaths = context.options[0] && context.options[0].ignoreNestedPaths;
const useAt = context.options[0] && context.options[0].useAt;
const useOptionalChaining = context.options[0] && context.options[0].useOptionalChaining;
const catchSafeObjects = context.options[0] ? context.options[0].catchSafeObjects : true;
const catchUnsafeObjects = context.options[0] && context.options[0].catchUnsafeObjects;
Expand Down Expand Up @@ -295,6 +305,7 @@ module.exports = {
path: node.arguments[0].value,
isImportedGet: false,
useOptionalChaining,
useAt,
objectText: sourceCode.getText(node.callee.object),
});
}
Expand All @@ -315,6 +326,7 @@ module.exports = {
path: node.arguments[1].value,
isImportedGet: true,
useOptionalChaining,
useAt,
objectText: sourceCode.getText(node.arguments[0]),
});
}
Expand Down Expand Up @@ -458,6 +470,7 @@ function validateGetPropertiesArguments(args, ignoreNestedPaths) {

function convertLiteralTypePath({
path,
useAt,
useOptionalChaining,
shouldIgnoreOptionalChaining,
objectText,
Expand All @@ -476,8 +489,8 @@ function convertLiteralTypePath({
return null;
}

if (path.match(/lastObject/g)?.length > 1) {
// Do not autofix when multiple `lastObject` are chained.
if (path.match(/lastObject/g)?.length > 1 && !useAt) {
// Do not autofix when multiple `lastObject` are chained, and use `at(-1)` is not allowed.
return null;
}

Expand All @@ -493,20 +506,27 @@ function convertLiteralTypePath({
replacementPath = replacementPath
.replace(/\.firstObject/g, shouldIgnoreOptionalChaining ? '[0]' : '.[0]') // When `firstObject` is used in the middle of the path. e.g. foo.firstObject
.replace(/^firstObject\??\./, shouldIgnoreOptionalChaining ? '[0].' : '[0]?.') // When `firstObject` is used at the beginning of the path. e.g. firstObject.bar
.replace(/^firstObject$/, '[0]') // When `firstObject` is used as the entire path.
.replace(
/\??\.lastObject/, // When `lastObject` is used in the middle of the path. e.g. foo.lastObject
(_, offset) =>
`${shouldIgnoreOptionalChaining ? '' : '?.'}[${objectText}.${replacementPath.slice(
0,
offset
)}.length - 1]`
)
.replace(
/^lastObject\??\./, // When `lastObject` is used at the beginning of the path. e.g. lastObject.bar
`[${objectText}.length - 1]${shouldIgnoreOptionalChaining ? '.' : '?.'}`
)
.replace(/^lastObject$/, `[${objectText}.length - 1]`); // When `lastObject` is used as the entire path.
.replace(/^firstObject$/, '[0]'); // When `firstObject` is used as the entire path.

// eslint-disable-next-line unicorn/prefer-ternary
if (useAt) {
replacementPath = replacementPath.replace(/lastObject/g, 'at(-1)');
} else {
replacementPath = replacementPath
.replace(
/\??\.lastObject/, // When `lastObject` is used in the middle of the path. e.g. foo.lastObject
(_, offset) =>
`${shouldIgnoreOptionalChaining ? '' : '?.'}[${objectText}.${replacementPath.slice(
0,
offset
)}.length - 1]`
)
.replace(
/^lastObject\??\./, // When `lastObject` is used at the beginning of the path. e.g. lastObject.bar
`[${objectText}.length - 1]${shouldIgnoreOptionalChaining ? '.' : '?.'}`
)
.replace(/^lastObject$/, `[${objectText}.length - 1]`); // When `lastObject` is used as the entire path.
}

const objectPathSeparator = replacementPath.startsWith('[') ? '' : '.';

Expand Down
100 changes: 85 additions & 15 deletions tests/lib/rules/no-get.js
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,19 @@ ruleTester.run('no-get', rule, {
},
],
},
{
// `firstObject` used in multiple places in a path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('firstObject.foo.firstObject.bar')[123];",
output: 'this[0].foo[0].bar[123];',
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// `firstObject` used in the middle of a path.
// And the resolved path of `get` is NOT chained (getResultIsChained=false).
Expand Down Expand Up @@ -765,8 +778,8 @@ ruleTester.run('no-get', rule, {
// `lastObject` used in the middle of a path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('foo.lastObject.bar')[123];",
output: 'this.foo[this.foo.length - 1].bar[123];',
options: [{ useOptionalChaining: true }],
output: 'this.foo.at(-1).bar[123];',
options: [{ useOptionalChaining: true, useAt: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
Expand All @@ -778,8 +791,8 @@ ruleTester.run('no-get', rule, {
// `lastObject` used at the beginning of a path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('lastObject.bar')[123];",
output: 'this[this.length - 1].bar[123];',
options: [{ useOptionalChaining: true }],
output: 'this.at(-1).bar[123];',
options: [{ useOptionalChaining: true, useAt: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
Expand All @@ -791,8 +804,8 @@ ruleTester.run('no-get', rule, {
// `lastObject` used as the entire path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('lastObject')[123];",
output: 'this[this.length - 1][123];',
options: [{ useOptionalChaining: true }],
output: 'this.at(-1)[123];',
options: [{ useOptionalChaining: true, useAt: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
Expand All @@ -804,8 +817,8 @@ ruleTester.run('no-get', rule, {
// `lastObject` used in the middle of a path.
// And the resolved path of `get` is NOT chained (getResultIsChained=false).
code: "this.get('foo.lastObject.bar');",
output: 'this.foo?.[this.foo.length - 1]?.bar;',
options: [{ useOptionalChaining: true }],
output: 'this.foo?.at(-1)?.bar;',
options: [{ useOptionalChaining: true, useAt: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
Expand All @@ -817,8 +830,8 @@ ruleTester.run('no-get', rule, {
// `lastObject` used at the beginning of a path.
// And the resolved path of `get` is NOT chained (getResultIsChained=false).
code: "this.get('lastObject.bar');",
output: 'this[this.length - 1]?.bar;',
options: [{ useOptionalChaining: true }],
output: 'this.at(-1)?.bar;',
options: [{ useOptionalChaining: true, useAt: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
Expand All @@ -827,11 +840,11 @@ ruleTester.run('no-get', rule, {
],
},
{
// `lastObject` used in the middle of a path.
// When multiple `lastObject` are chained, it won't auto-fix.
// multiple `lastObject` used in the middle of a path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('foo.lastObject.bar.lastObject')[123];",
output: null,
options: [{ useOptionalChaining: true }],
output: 'this.foo.at(-1).bar.at(-1)[123];',
options: [{ useOptionalChaining: true, useAt: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
Expand All @@ -843,8 +856,65 @@ ruleTester.run('no-get', rule, {
// `lastObject` used at the beginning of a path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('lastObject.bar.lastObject')[123];",
output: 'this.at(-1).bar.at(-1)[123];',
options: [{ useOptionalChaining: true, useAt: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},

{
// useAt = false.
// `lastObject` used at the beginning of a path.
// And the resolved path of `get` is NOT chained (getResultIsChained=false).
code: "this.get('lastObject.bar');",
output: 'this[this.length - 1]?.bar;',
options: [{ useOptionalChaining: true, useAt: false }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// useAt = false.
// `lastObject` used as the entire path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('lastObject')[123];",
output: 'this[this.length - 1][123];',
options: [{ useOptionalChaining: true, useAt: false }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// useAt = false.
// multiple `lastObject` used in the middle of a path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('foo.lastObject.bar.lastObject')[123];",
output: null,
options: [{ useOptionalChaining: true }],
options: [{ useOptionalChaining: true, useAt: false }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// useAt = false.
// `lastObject` used in the middle of a path.
// And the resolved path of `get` is NOT chained (getResultIsChained=false).
code: "this.get('foo.lastObject.bar');",
output: 'this.foo?.[this.foo.length - 1]?.bar;',
options: [{ useOptionalChaining: true, useAt: false }],
errors: [
{
message: ERROR_MESSAGE_GET,
Expand Down