feat(eslint-plugin): [sort-lifecycle-methods] add rule #1320

Friendseeker · 2023-03-06T00:15:45Z

Implements feature requested by #995

Add feature requested by angular-eslint#995

nx-cloud · 2023-03-06T00:15:47Z

☁️ Nx Cloud Report

We didn't find any information for the current pull request with the commit 67abe2f.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.

Sent with 💌 from NxCloud.

…hods position

Friendseeker · 2023-03-06T17:01:30Z

The issue I had when I worked on the implementation is that, it is relatively easy to determine if methods are not in right order, but it can be hard to determine which methods should be reported... (e.g. Say method B, C are erroneously placed before method A, should B be reported, C be reported, A be reported, or both B, C be reported, ..., or the class instead of individual methods should be reported)

Which one way of reporting should be adopted, or is it okay to report in whichever way that is convenient to implement (e.g. the current way)?

Friendseeker · 2023-03-07T01:27:37Z

TODO:

@Input() set methods should be allowed to be declared before lifecycle methods.
Constructor should be before lifecycle methods

Ordering should be:

@Inputs()
Constructor
Lifecycle methods
Non-Lifecycle methods

JamesHenry · 2023-04-05T15:18:10Z

packages/eslint-plugin/src/rules/sort-lifecycle-methods.ts

+import { ASTUtils, Selectors } from '@angular-eslint/utils';
+import type { TSESTree } from '@typescript-eslint/utils';
+import { createESLintRule } from '../utils/create-eslint-rule';
+import type { AngularLifecycleMethods } from '@angular-eslint/utils/dist/eslint-plugin/ast-utils';


It's unexpected to have a deep import here, is the symbol not exported?

JamesHenry · 2023-04-05T15:19:26Z

Thanks a lot for working on this @Friendseeker!

fandy · 2023-04-20T15:04:26Z

Any updates on this? ❤️

Friendseeker · 2023-04-30T00:50:49Z

Any updates on this? ❤️

Sorry. I have been especially busy. I will work on it if I have spare time (If I... will have any spare time).

abaran30 · 2023-05-15T17:27:04Z

packages/utils/src/eslint-plugin/ast-utils.ts

@@ -91,6 +91,16 @@ export const angularLifecycleInterfaceKeys = objectKeys(
 );
 export const angularLifecycleMethodKeys = objectKeys(AngularLifecycleMethods);



Would you be able to include a comment containing the URL to the lifecycle event sequence?

https://angular.io/guide/lifecycle-hooks#lifecycle-event-sequence

Done. See below.

jerone · 2023-05-18T11:44:24Z

packages/eslint-plugin/src/rules/sort-lifecycle-methods.ts

+              node: previous.key,
+              messageId: 'nonLifecycleMethodBeforeLifecycleMethod',
+            });
+          }


You could limit to one for loop by adding an extra condition here, and discarding the second loop.

Pseudo-code:

let previousLifecycleMethod = null; // Declare outside loop. if (isLifecycleMethod(current) && previousLifecycleMethod != null) { if (isBefore(current, previousLifecycleMethod)) { context.report({ node: current.key, messageId: 'lifecycleMethodsNotSorted', }); } previousLifecycleMethod = current; }

@jerone I considered this. However the first loop looks at all methods, checks the placement of the constructor and non-lifecycle methods. The second loop targets only the lifecycle methods and checks sequence. I added comments for clarity.

Understood. The code change significantly since making this suggestion.

JamesHenry · 2023-06-01T08:16:17Z

Converted to draft to more accurately reflect its current state

sc0tt5 · 2023-07-23T17:48:18Z

@Friendseeker @JamesHenry I would like to contribute to this PR if possible. Please advise.

Friendseeker · 2023-07-23T17:59:11Z

@Friendseeker @JamesHenry I would like to contribute to this PR if possible. Please advise.

Hi @sc0tt5! Sorry that it took me so long but I finally regained some spare time during weekends (for which I can use to wrap up this PR). Would definitely appreciate you to work on this PR to wrap it up!

First off, would you say my below comment is reflective on what this feature should be? When I first worked on this feature, I did not consider all the cases, and I am still not sure if all cases are considered in the comment below.

TODO:

@Input() set methods should be allowed to be declared before lifecycle methods.

Constructor should be before lifecycle methods

Ordering should be:

@Inputs()

Constructor

Lifecycle methods

Non-Lifecycle methods

Also, if order is incorrect, which methods should be flagged? Should the entire class be flagged inside? I still don't have a good idea on that problem, and would really appreciate your feedback.

sc0tt5 · 2023-07-23T18:10:54Z

Hello @Friendseeker! No worries. I was delighted to find someone had taken the initiative to work this in. I will double check the ordering and anything we might be missing, then reconfirm with you soon. Give me until later today/tonight.

Then once we're on the same page, I'll send over my contribution sometime soon. How should I contribute? I assume PR into your branch?

Friendseeker · 2023-07-23T18:14:03Z

Hello @Friendseeker! No worries. I was delighted to find someone had taken the initiative to work this in. I will double check the ordering and anything we might be missing, then reconfirm with you soon. Give me until later today/tonight.

Then once we're on the same page, I'll send over my contribution sometime soon. How should I contribute? I assume PR into your branch?

Really appreciate, @sc0tt5 ! Feel free to take as much time as you need! Better to consider all cases than rushing it and having to redo things afterwards.

PR into my branch sounds great!

…e-methods

Update from angular-eslint main

sc0tt5 · 2023-08-01T23:37:37Z

@JamesHenry please consider removing the draft status and review the recent updates that I submitted via @Friendseeker's PR.

abaran30 · 2023-08-06T20:02:36Z

packages/eslint-plugin/tests/rules/sort-lifecycle-methods/cases.ts

+      `,
+    messageId: lifecycleMethodBeforeConstructor,
+  }),
+];


What's the expected behavior if more than one lifecycle hook is in the incorrect order?

@sc0tt5 @abaran30 Should we find all inversions and flag all lifecycle methods in the inversions?

e.g. For

ngOnInit ngOnChanges ngDoCheck

The inversion pair is (ngOnInit, ngOnChanges) so we flag both methods.

Or can we find a more optimal (yet deterministic) way to flag methods?

I'm assuming that with the following example...

ngOnInit() {} ngOnChanges() {} ngAfterContentInit() {} ngDoCheck() {}

...this rule will report twice: first on ngOnChanges and then on ngDoCheck. If my assumption is correct, I think this approach is appropriate.

Thanks for the feedback! Just want to clarify, in the example

ngOnInit() {} ngOnChanges() {} ngAfterContentInit() {} ngDoCheck() {}

There's two inversion pairs (ngOnInit, ngOnChanges) and (ngAfterContentInit, ngDoCheck). The agreed rule is for each inversion pair, we flag the second element in the pair.

And going back to

ngOnInit ngOnChanges ngDoCheck

Since the inversion pair is (ngOnInit, ngOnChanges), only ngOnChanges should be flagged.

Thank you for clarifying. Makes sense to me 🙂.

One thing to keep in mind with this approach and if there will be an auto-fixer for this rule in the future... The ESLint fix "process will repeat up to 10 times, or until no more fixable problems are found," as documented here.

Should be okay for this rule because there are only 8 Angular lifecycle events, at least for now.

If possible, maybe let's play around with test cases that check for multiple errors? See here.

@sc0tt5 What is your opinion on inversion based flagging?

abaran30 · 2023-08-06T20:12:50Z

@Friendseeker @JamesHenry I would like to contribute to this PR if possible. Please advise.

Hi @sc0tt5! Sorry that it took me so long but I finally regained some spare time during weekends (for which I can use to wrap up this PR). Would definitely appreciate you to work on this PR to wrap it up!

First off, would you say my below comment is reflective on what this feature should be? When I first worked on this feature, I did not consider all the cases, and I am still not sure if all cases are considered in the comment below.

TODO:

@Input() set methods should be allowed to be declared before lifecycle methods.

Constructor should be before lifecycle methods

Ordering should be:

@Inputs()

Constructor

Lifecycle methods

Non-Lifecycle methods

Also, if order is incorrect, which methods should be flagged? Should the entire class be flagged inside? I still don't have a good idea on that problem, and would really appreciate your feedback.

I'm curious why there's a consideration for where the lifecycle method implementations are, e.g. before or after the constructor and other methods. IMO, I'd expect this rule to only care about the sort order of the lifecycle methods and not where they are implemented, even though I can imagine most developers preferring some order like suggested in the quoted comment. Perhaps I'm missing something from the discussions or established expectations?

Friendseeker · 2023-08-06T20:21:16Z

@Friendseeker @JamesHenry I would like to contribute to this PR if possible. Please advise.

Hi @sc0tt5! Sorry that it took me so long but I finally regained some spare time during weekends (for which I can use to wrap up this PR). Would definitely appreciate you to work on this PR to wrap it up!
First off, would you say my below comment is reflective on what this feature should be? When I first worked on this feature, I did not consider all the cases, and I am still not sure if all cases are considered in the comment below.

TODO:

@Input() set methods should be allowed to be declared before lifecycle methods.

Constructor should be before lifecycle methods

Ordering should be:

@Inputs()

Constructor

Lifecycle methods

Non-Lifecycle methods

Also, if order is incorrect, which methods should be flagged? Should the entire class be flagged inside? I still don't have a good idea on that problem, and would really appreciate your feedback.

I'm curious why there's a consideration for where the Lifecycle method implementations are, e.g. before or after the constructor and other methods. IMO, I'd expect this rule to only care about the sort order of the Lifecycle methods and not where they are implemented, even though I can imagine most developers preferring some order like suggested in the quoted comment. Perhaps I'm missing something from the discussions or established expectations?

Appreciate your feedback!

I did some thinking and I personally think you are right. I overcomplicated the problem. The rule should have one single responsibility only, being the lifecycles methods themselves are in order. Other method's ordering do not matter.

@sc0tt5 What do you think?

sc0tt5 · 2023-08-06T21:30:15Z

@Friendseeker yes, sounds good to me. @abaran30 I'm happy to revert changes referencing the constructor, or wait on updates from you before making any changes. Please let me know.

abaran30 · 2023-08-06T22:07:33Z

@sc0tt5 let's make the rule focus on the sorting of lifecycle methods only. 🙂

bevrard · 2023-08-07T06:57:22Z

The proposal with sort and position was just what I was looking for ;-) How could we do it if it's not included with this rule?

Friendseeker · 2023-08-07T19:29:32Z

The proposal with sort and position was just what I was looking for ;-) How could we do it if it's not included with this rule?

Likely via a combination of multiple rules.

jerone · 2023-08-07T19:42:32Z

The proposal with sort and position was just what I was looking for ;-) How could we do it if it's not included with this rule?

Likely via a combination of multiple rules.

Sounds more like an overload for @typescript-eslint/member-ordering.

Similar request: #1424

sc0tt5 · 2023-08-08T04:18:41Z

I like where we landed with this, only focused on sort order of the lifecycle methods. It's concise and serves it's purpose within the Angular scope of things. I did test with @typescript-eslint/member-ordering and as @bevrard mentioned it would be nice to somehow have these positioned, which is out of scope here. It doesn't seem possible with @typescript-eslint/member-ordering. I will probably try a custom rule in my project.

chore: focus on sorting lifecycle methods only

nx-cloud · 2023-09-04T13:19:30Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2d9090c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 7 targets

Sent with 💌 from NxCloud.

codecov · 2023-09-04T13:27:05Z

Codecov Report

Merging #1320 (2d9090c) into main (2e5ed71) will increase coverage by 0.05%.
Report is 1 commits behind head on main.
The diff coverage is 95.83%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1320      +/-   ##
==========================================
+ Coverage   89.23%   89.28%   +0.05%     
==========================================
  Files         162      164       +2     
  Lines        3056     3081      +25     
  Branches      521      523       +2     
==========================================
+ Hits         2727     2751      +24     
  Misses        201      201              
- Partials      128      129       +1

Flag	Coverage Δ
unittest	`89.28% <95.83%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
packages/eslint-plugin/src/index.ts	`70.17% <ø> (+0.53%)`	⬆️
.../eslint-plugin/src/rules/sort-lifecycle-methods.ts	`94.44% <94.44%> (ø)`
...plugin/tests/rules/sort-lifecycle-methods/cases.ts	`100.00% <100.00%> (ø)`

JamesHenry

Thanks everyone for their hard work and consideration on this one! I think we can ship it

…t#1320)

feat(eslint-plugin): [sort-lifecycle-methods]

76f1e35

Add feature requested by angular-eslint#995

feat(eslint-plugin): [sort-lifecycle-methods] check non-lifecycle met…

9f46d5b

…hods position

Friendseeker changed the title ~~feat(eslint-plugin): [sort-lifecycle-methods]~~ feat(eslint-plugin): [sort-lifecycle-methods] add rule Mar 6, 2023

Friendseeker added 3 commits March 5, 2023 21:11

feat(eslint-plugin): [sort-lifecycle-methods] update doc

81a1347

feat(eslint-plugin): [sort-lifecycle-methods] update index.ts & all.json

6ef3a32

feat(eslint-plugin): [sort-lifecycle-methods] disable recommended

67abe2f

JamesHenry reviewed Apr 5, 2023

View reviewed changes

abaran30 reviewed May 15, 2023

View reviewed changes

jerone reviewed May 18, 2023

View reviewed changes

JamesHenry marked this pull request as draft June 1, 2023 08:16

Merge remote-tracking branch 'angular-eslint/main' into sort-lifecycl…

6d5e6cc

…e-methods

sc0tt5 mentioned this pull request Jul 23, 2023

Update from angular-eslint main Friendseeker/angular-eslint#1

Merged

sc0tt5 and others added 5 commits July 23, 2023 17:47

chore: remove duplicate key after merge

1b0588d

Merge pull request #1 from sc0tt5/sort-lifecycle-methods

6bcfaf1

Update from angular-eslint main

chore: comment url lifecycle hook sequence

51c2288

chore: update rule to consider constructor

239d2b0

Merge pull request #2 from sc0tt5/sort-lifecycle-methods

c5a780b

abaran30 reviewed Aug 6, 2023

View reviewed changes

chore: focus on sorting lifecycle methods only

9849c2a

sc0tt5 mentioned this pull request Aug 8, 2023

chore: focus on sorting lifecycle methods only Friendseeker/angular-eslint#3

Merged

sc0tt5 and others added 2 commits August 10, 2023 20:48

chore: sync remote main

2627686

Merge pull request #3 from sc0tt5/sort-lifecycle-methods

0db6a30

chore: focus on sorting lifecycle methods only

Friendseeker marked this pull request as ready for review August 14, 2023 05:06

joergbaier mentioned this pull request Aug 16, 2023

feat(eslint-plugin): new rule sort-imports-metadata #1303

Closed

Merge branch 'main' into sort-lifecycle-methods

2d9090c

JamesHenry approved these changes Sep 16, 2023

View reviewed changes

JamesHenry merged commit 47f7975 into angular-eslint:main Sep 16, 2023
14 checks passed

strawberry-choco pushed a commit to strawberry-choco/angular-eslint that referenced this pull request Sep 17, 2023

feat(eslint-plugin): [sort-lifecycle-methods] add rule (angular-eslin…

ec0ded5

…t#1320)

Samuel-Therrien-Beslogic mentioned this pull request Nov 6, 2023

[sort-lifecycle-methods] Sort Angular Lifecycle Methods #995

Open

		@@ -91,6 +91,16 @@ export const angularLifecycleInterfaceKeys = objectKeys(
		);
		export const angularLifecycleMethodKeys = objectKeys(AngularLifecycleMethods);

feat(eslint-plugin): [sort-lifecycle-methods] add rule #1320

feat(eslint-plugin): [sort-lifecycle-methods] add rule #1320

Conversation

Friendseeker commented Mar 6, 2023 • edited

nx-cloud bot commented Mar 6, 2023 • edited

☁️ Nx Cloud Report

Friendseeker commented Mar 6, 2023 • edited

Friendseeker commented Mar 7, 2023 • edited

JamesHenry Apr 5, 2023

Choose a reason for hiding this comment

JamesHenry commented Apr 5, 2023

fandy commented Apr 20, 2023

Friendseeker commented Apr 30, 2023

abaran30 May 15, 2023

Choose a reason for hiding this comment

sc0tt5 Aug 1, 2023

Choose a reason for hiding this comment

jerone May 18, 2023 • edited

Choose a reason for hiding this comment

sc0tt5 Aug 1, 2023 • edited

Choose a reason for hiding this comment

jerone Aug 3, 2023

Choose a reason for hiding this comment

JamesHenry commented Jun 1, 2023

sc0tt5 commented Jul 23, 2023

Friendseeker commented Jul 23, 2023 • edited

sc0tt5 commented Jul 23, 2023

Friendseeker commented Jul 23, 2023

sc0tt5 commented Aug 1, 2023 • edited

abaran30 Aug 6, 2023

Choose a reason for hiding this comment

Friendseeker Aug 6, 2023 • edited

Choose a reason for hiding this comment

abaran30 Aug 6, 2023 • edited

Choose a reason for hiding this comment

Friendseeker Aug 7, 2023 • edited

Choose a reason for hiding this comment

abaran30 Aug 7, 2023

Choose a reason for hiding this comment

abaran30 Aug 7, 2023 • edited

Choose a reason for hiding this comment

Friendseeker Aug 7, 2023 • edited

Choose a reason for hiding this comment

abaran30 commented Aug 6, 2023 • edited

Friendseeker commented Aug 6, 2023 • edited

sc0tt5 commented Aug 6, 2023

abaran30 commented Aug 6, 2023

bevrard commented Aug 7, 2023

Friendseeker commented Aug 7, 2023 • edited

jerone commented Aug 7, 2023 • edited

sc0tt5 commented Aug 8, 2023

nx-cloud bot commented Sep 4, 2023 • edited

☁️ Nx Cloud Report

codecov bot commented Sep 4, 2023 • edited

Codecov Report

JamesHenry left a comment

Choose a reason for hiding this comment

Friendseeker commented Mar 6, 2023 •

edited

nx-cloud bot commented Mar 6, 2023 •

edited

Friendseeker commented Mar 6, 2023 •

edited

Friendseeker commented Mar 7, 2023 •

edited

jerone May 18, 2023 •

edited

sc0tt5 Aug 1, 2023 •

edited

Friendseeker commented Jul 23, 2023 •

edited

sc0tt5 commented Aug 1, 2023 •

edited

Friendseeker Aug 6, 2023 •

edited

abaran30 Aug 6, 2023 •

edited

Friendseeker Aug 7, 2023 •

edited

abaran30 Aug 7, 2023 •

edited

Friendseeker Aug 7, 2023 •

edited

abaran30 commented Aug 6, 2023 •

edited

Friendseeker commented Aug 6, 2023 •

edited

Friendseeker commented Aug 7, 2023 •

edited

jerone commented Aug 7, 2023 •

edited

nx-cloud bot commented Sep 4, 2023 •

edited

codecov bot commented Sep 4, 2023 •

edited