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.

Sign up for GitHub

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

Jump to bottom

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

Add requireArgs option in require-super-in-lifecycle-hooks rule #1836

wants to merge 6 commits into from

Conversation

samridhivig
Copy link

@samridhivig samridhivig commented Apr 17, 2023 •
edited
Loading

Fix #1784:

Updated require-super-in-lifecycle-hooks rule to error when super.init() is called without ...arguments.

@samridhivig samridhivig changed the title #1784: Ensure super() called with args in init hook Fix #1784: Ensure super() called with args in init hook Apr 17, 2023
@samridhivig samridhivig changed the title Fix #1784: Ensure super() called with args in init hook Ensure super() called with args in init hook Apr 17, 2023
@samridhivig samridhivig mentioned this pull request Apr 17, 2023
Open
bmish
bmish requested changes Apr 17, 2023
View reviewed changes
Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

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

We'll need to see some test cases to better understand what this change is doing..

lib/rules/require-super-in-lifecycle-hooks.js Outdated Show resolved Hide resolved
@samridhivig samridhivig marked this pull request as draft April 17, 2023 18:49
@samridhivig samridhivig marked this pull request as ready for review April 17, 2023 21:11
@bmish
Copy link
Member

bmish commented Apr 17, 2023

We'll also need to put this behind an option (e.g. requireArgs) to avoid this being a breaking change. In a future major version, we could enable the option by default.

@bmish bmish changed the title Ensure super() called with args in init hook Add requireArgs option in require-super-in-lifecycle-hooks Apr 18, 2023
@bmish bmish changed the title Add requireArgs option in require-super-in-lifecycle-hooks Add requireArgs option in require-super-in-lifecycle-hooks rule Apr 18, 2023
bmish
bmish reviewed Apr 18, 2023
View reviewed changes
Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

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

Just wondering, why are you only targeting init and not other lifecycle hooks?

lib/rules/require-super-in-lifecycle-hooks.js Outdated
).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.

tests/lib/rules/require-super-in-lifecycle-hooks.js
@@ -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 •
edited
Loading

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

lib/rules/require-super-in-lifecycle-hooks.js Outdated Show resolved Hide resolved
@samridhivig
Copy link
Author

@bmish Regarding your question, "Just wondering, why are you only targeting init and not other lifecycle hooks?"

Currently, I only added the requireArgs option for init hook as explicitly mentioned in the parent issue: ef4/ember-data-relationship-tracker#25 (review)

I didn't expand it to other hooks as I am not sure if all other hooks prefer to have ...arguments as well. If other hooks don't, then enforcing this linting rule for them can lead to verbose code.

@bmish
Copy link
Member

bmish commented Apr 18, 2023

@samridhivig I see. Yeah so we only know about the Ember Data issue with init needing args right now, but would you mind asking on the Ember Discord or otherwise doing some quick research to see if the other hooks should technically have args passed as well? I just want to avoid the situation where we need another breaking change later if we discover that all hooks need args. It also seems that we might as well require args for all hooks for consistency...which would also be helpful in case the other hooks end up needing args in the future.

@NullVoxPopuli
Copy link
Contributor

Where's this rule at? Close?
Looks like there are some unresolved questions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmish bmish bmish requested changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ensure super() called with args in hooks
3 participants
@samridhivig @bmish @NullVoxPopuli