fix: ng-add should also work for ESLint 9 flat config files (#4643) #4698

adxdits · 2025-02-09T14:46:32Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Today: When someone runs the NgRx ESLint schematic (for example via an ng add command), it updates the ESLint configuration by editing a JSON file (like .eslintrc.json) to add the NgRx plugin and its rules.
Closes #4643

What is the new behavior?

Detect if eslint.config.js exists and update it with NgRx plugin
Fallback to .eslintrc.json when flat config is missing
Preserve existing configuration

Steps to push the change:

Does this PR introduce a breaking change?

Yes
No

Other information

netlify · 2025-02-09T14:46:49Z

✅ Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`acaac29`
🔍 Latest deploy log	https://app.netlify.com/sites/ngrx-io/deploys/67bb878ddb69600008790837

timdeschryver

Hello, thanks for this PR ~ I will review this later this week.
I already noticed that there are no test cases, could you please write some tests to cover these changes?
I'm thinking of:

a test to detect the correct file is changed
a test that adds the NgRx plugin to a flat config file
a test that verifies the plugin isn't added twice

timdeschryver · 2025-02-11T19:28:54Z

modules/eslint-plugin/schematics/ng-add/index.ts

@@ -4,17 +4,79 @@ import type { Schema } from './schema';

 export default function addNgRxESLintPlugin(schema: Schema): Rule {
  return (host: Tree, context: SchematicContext) => {
-    const eslintConfigPath = '.eslintrc.json';
+    const jsonConfigPath = '.eslintrc.json';
+    const flatConfigPath = 'eslint.config.js';


This should also cover commonjs and modulejs files:

eslint.config.js

eslint.config.mjs

eslint.config.cjs

should we also support .eslintrc.(m|c)js?

@markostanimirovic I don't think so, those aren't supported anymore.
https://eslint.org/docs/latest/use/configure/configuration-files#configuration-file

just as @timdeschryver said i updated the schematics to support only the flat ESLint config files (eslint.config.js, eslint.config.mjs, and eslint.config.cjs) since .eslintrc.(m|c)js files are no longer supported. :)

I only see eslint.config.js here, could you also want to include the other extensions?

adxdits · 2025-02-12T08:46:48Z

Hello, thanks for this PR ~ I will review this later this week. I already noticed that there are no test cases, could you please write some tests to cover these changes? I'm thinking of:

a test to detect the correct file is changed

a test that adds the NgRx plugin to a flat config file

a test that verifies the plugin isn't added twice

Hello, of course. Thanks for pointing that out!

timdeschryver

C

timdeschryver · 2025-02-16T16:12:59Z

modules/eslint-plugin/schematics/ng-add/index.ts

+
+      // Add the import for the NgRx ESLint plugin if not already present.
+      if (!content.includes("from '@ngrx/eslint-plugin'")) {
+        content = `import ngrxEslintPlugin from '@ngrx/eslint-plugin';\n` + content;


Is it possible to match this with the current import statements?
For example, when it's not the module syntax we should use

const ngrx = require("@ngrx/eslint-plugin");

timdeschryver · 2025-02-16T16:26:00Z

modules/eslint-plugin/schematics/ng-add/index.ts

+      // Check if the flat config already contains an NgRx config.
+      if (!content.includes('plugin:@ngrx')) {
+        // Look for an exported default array.
+        const exportDefaultRegex = /export\s+default\s+\[([\s\S]*?)\];/;


Same comment here, ideally we also support module.exports = ....
As I expect most eslint configs use tseslint, checking for tseslint.config is probably the simplest solution. Updating it is harder than a JSON file, I think it can be easier if we use the TS AST to find the tseslint.config CallExpression, and add an argument.

timdeschryver · 2025-02-16T16:27:18Z

modules/eslint-plugin/schematics/ng-add/index.ts

-    const eslint = host.read(eslintConfigPath)?.toString('utf-8');
+    // If a flat config exists, update it
+    if (host.exists(flatConfigPath)) {
+      let content = host.read(flatConfigPath)?.toString('utf-8');


This method is becoming lengthy, could you split it up into at least two methods, one for the flat config, the other for the JSON file?

…grx#4643)

…lint.config update

timdeschryver · 2025-02-21T18:48:55Z

modules/eslint-plugin/schematics/ng-add/index.ts

@@ -4,17 +4,79 @@ import type { Schema } from './schema';

 export default function addNgRxESLintPlugin(schema: Schema): Rule {
  return (host: Tree, context: SchematicContext) => {
-    const eslintConfigPath = '.eslintrc.json';
+    const jsonConfigPath = '.eslintrc.json';
+    const flatConfigPath = 'eslint.config.js';


I only see eslint.config.js here, could you also want to include the other extensions?

modules/eslint-plugin/schematics/ng-add/index.ts

…date

timdeschryver

It seems as if I can't push to your branch, please update the files with https://gist.github.com/timdeschryver/bb49c998c5a39ebe289b77412aa8fd0f.
The main changes are to use Angular's recorder instead of TypeScript (ts.factory) to make the changes, this keeps the implementation consistent with our codebase.
It also updates the tests.
For the next time, please make sure the tests are green while creating a Pull Requests.

Also, please revert the changes to the package and package-lock files.

…date tests, revert package files

fix: ng-add should also work for ESLint 9 flat config files (ngrx#4643)

cbf6341

timdeschryver requested changes Feb 11, 2025

View reviewed changes

adxdits and others added 2 commits February 12, 2025 11:08

test(eslint-plugin): add tests for ESLint plugin schematics

7b513ba

Merge branch 'main' into main

4f53d28

timdeschryver requested changes Feb 16, 2025

View reviewed changes

adxdits and others added 2 commits February 17, 2025 14:50

fix(ng-add): ng-add should also work for ESLint 9 flat config files (n…

ceed1cf

…grx#4643)

feat(eslint-plugin): split config update logic and add AST-based tses…

75cb34a

…lint.config update

adxdits requested a review from timdeschryver February 21, 2025 12:38

timdeschryver reviewed Feb 21, 2025

View reviewed changes

fix(eslint-plugin): add missing closing parenthesis in JSON config up…

076c606

…date

adxdits requested a review from timdeschryver February 21, 2025 22:29

timdeschryver requested changes Feb 23, 2025

View reviewed changes

refactor(eslint-plugin): replace ts.factory with Angular recorder, up…

acaac29

…date tests, revert package files

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ng-add should also work for ESLint 9 flat config files (#4643) #4698

fix: ng-add should also work for ESLint 9 flat config files (#4643) #4698

adxdits commented Feb 9, 2025

netlify bot commented Feb 9, 2025 •

edited

Loading

timdeschryver left a comment

timdeschryver Feb 11, 2025

markostanimirovic Feb 11, 2025

timdeschryver Feb 12, 2025 •

edited

Loading

adxdits Feb 12, 2025

timdeschryver Feb 21, 2025

adxdits commented Feb 12, 2025

timdeschryver left a comment

timdeschryver Feb 16, 2025

timdeschryver Feb 16, 2025

timdeschryver Feb 16, 2025

timdeschryver Feb 21, 2025

timdeschryver left a comment

fix: ng-add should also work for ESLint 9 flat config files (#4643) #4698

Are you sure you want to change the base?

fix: ng-add should also work for ESLint 9 flat config files (#4643) #4698

Conversation

adxdits commented Feb 9, 2025

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

netlify bot commented Feb 9, 2025 • edited Loading

✅ Deploy Preview for ngrx-io canceled.

timdeschryver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdeschryver Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adxdits commented Feb 12, 2025

timdeschryver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdeschryver left a comment

Choose a reason for hiding this comment

netlify bot commented Feb 9, 2025 •

edited

Loading

timdeschryver Feb 12, 2025 •

edited

Loading