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 requireArgs option in require-super-in-lifecycle-hooks rule #1836

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 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
9 changes: 5 additions & 4 deletions docs/rules/require-super-in-lifecycle-hooks.md
Expand Up @@ -20,7 +20,7 @@ import Component from '@ember/component';
export default Component.extend({
init() {
this.set('items', []);
}
},
});
```

Expand All @@ -30,7 +30,7 @@ import Component from '@ember/component';
export default Component.extend({
didInsertElement() {
// ...
}
},
});
```

Expand All @@ -53,7 +53,7 @@ export default Component.extend({
init(...args) {
this._super(...args);
this.set('items', []);
}
},
});
```

Expand All @@ -64,7 +64,7 @@ export default Component.extend({
didInsertElement(...args) {
this._super(...args);
// ...
}
},
});
```

Expand Down Expand Up @@ -96,3 +96,4 @@ This rule takes an optional object containing:

- `boolean` -- `checkInitOnly` -- whether the rule should only check the `init` lifecycle hook and not other lifecycle hooks (default `false`)
- `boolean` -- `checkNativeClasses` -- whether the rule should check lifecycle hooks in native classes (in addition to classic classes) (default `true`)
- `boolean` -- `requireArgs` -- whether the rule should check if super() in init hook is called with args (default `false`)
50 changes: 42 additions & 8 deletions lib/rules/require-super-in-lifecycle-hooks.js
Expand Up @@ -6,15 +6,20 @@ const estraverse = require('estraverse');

function hasMatchingNode(node, matcher) {
let foundMatch = false;
let foundNode = null;
estraverse.traverse(node, {
enter(child) {
if (!foundMatch && matcher(child)) {
foundMatch = true;
foundNode = child;
}
},
fallback: 'iteration',
});
return foundMatch;
return {
foundMatch,
foundNode,
};
}

/**
Expand Down Expand Up @@ -79,6 +84,10 @@ module.exports = {
type: 'boolean',
default: true,
},
requireArgs: {
type: 'boolean',
default: false,
},
},
additionalProperties: false,
},
Expand All @@ -88,8 +97,9 @@ module.exports = {
create(context) {
const checkInitOnly = context.options[0] && context.options[0].checkInitOnly;
const checkNativeClasses = !context.options[0] || context.options[0].checkNativeClasses;
const requireArgs = context.options[0] && context.options[0].requireArgs;

function report(node, isNativeClass, lifecycleHookName) {
function report(node, isNativeClass, lifecycleHookName, hasSuper) {
context.report({
node,
message: ERROR_MESSAGE,
Expand All @@ -100,12 +110,23 @@ module.exports = {
: '...arguments';

const replacement = isNativeClass
? `super.${lifecycleHookName}(${replacementArgs});`
: `this._super(${replacementArgs});`;
? `super.${lifecycleHookName}(${replacementArgs})`
: `this._super(${replacementArgs})`;

// If super is already called, replace it with the correct call
if (hasSuper) {
const nodeToBeReplaced = hasMatchingNode(node, (bodyChild) =>
isNativeClass
? isNativeSuper(bodyChild, lifecycleHookName)
: isClassicSuper(bodyChild)
).foundNode;
return fixer.replaceText(nodeToBeReplaced, replacement);
}

// Insert right after function curly brace.
const sourceCode = context.getSourceCode();
const startOfBlockStatement = sourceCode.getFirstToken(node.value.body);
return fixer.insertTextAfter(startOfBlockStatement, `\n${replacement}`);
return fixer.insertTextAfter(startOfBlockStatement, `\n${replacement};`);
},
});
}
Expand Down Expand Up @@ -157,9 +178,22 @@ module.exports = {
const body = isNativeSuper ? node.value.body : node.body;
const hasSuper = hasMatchingNode(body, (bodyChild) =>
isNativeClass ? isNativeSuper(bodyChild, hookName) : isClassicSuper(bodyChild)
);
if (!hasSuper) {
report(node, isNativeClass, hookName);
).foundMatch;

if (hookName === 'init' && hasSuper && requireArgs) {
const hasSuperWithArguments = hasMatchingNode(
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid searching for the node again? We should already have it from the above hasMatchingNode call right, so we can just check if that node has arguments.

body,
(bodyChild) =>
(isNativeSuper(bodyChild, hookName) || isClassicSuper(bodyChild)) &&
bodyChild.arguments &&
bodyChild.arguments.length > 0
).foundMatch;

if (!hasSuperWithArguments) {
report(node, isNativeClass, hookName, hasSuper);
}
} else if (!hasSuper) {
report(node, isNativeClass, hookName, hasSuper);
}
}

Expand Down
81 changes: 81 additions & 0 deletions tests/lib/rules/require-super-in-lifecycle-hooks.js
Expand Up @@ -231,6 +231,19 @@ eslintTester.run('require-super-in-lifecycle-hooks', rule, {

// Does not throw with node type (ClassProperty) not handled by estraverse.
'Component.extend({init() { class Foo { @someDecorator() someProp }; return this._super(...arguments); } });',

// init hook should not be checked for arguments when requireArgs = false
Copy link
Member

@bmish bmish Apr 18, 2023

Choose a reason for hiding this comment

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

Let's include a test case for both:

  • implicit requireArgs = false (since it's the default)
  • explicit requireArgs = false

`import Service from '@ember/service';
class Foo extends Service {
init() {
super.init();
}
}`,
`export default Component.extend({
init() {
this._super(...arguments);
},
});`,
],
invalid: [
{
Expand Down Expand Up @@ -858,5 +871,73 @@ super.didInsertElement(...arguments);}
options: [{ checkNativeClasses: true, checkInitOnly: false }],
errors: [{ message, line: 3 }],
},

// init hooks shoudn't call super without ...arguments
{
code: `import Service from '@ember/service';
class Foo extends Service {
init() {
super.init();
}
}`,
output: `import Service from '@ember/service';
class Foo extends Service {
init() {
super.init(...arguments);
}
}`,
options: [{ requireArgs: true }],
errors: [{ message, line: 3 }],
},
{
code: `import Service from '@ember/service';
class Foo extends Service {
init() {
super.init();
console.log();
}
}`,
output: `import Service from '@ember/service';
class Foo extends Service {
init() {
super.init(...arguments);
console.log();
}
}`,
options: [{ requireArgs: true }],
errors: [{ message, line: 3 }],
},
{
code: `export default Component.extend({
init() {
this._super();
},
});`,
output: `export default Component.extend({
init() {
this._super(...arguments);
},
});`,
options: [{ requireArgs: true }],
errors: [{ message, line: 2 }],
},
{
code: `export default Component.extend({
init() {
this._super();
this.set('prop', 'value');
this.set('prop2', 'value2');
},
});`,
output: `export default Component.extend({
init() {
this._super(...arguments);
this.set('prop', 'value');
this.set('prop2', 'value2');
},
});`,
options: [{ requireArgs: true }],
errors: [{ message, line: 2 }],
},
],
});