-
-
Notifications
You must be signed in to change notification settings - Fork 2k
chore: migrate eslint config to flat format #4887
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for ngrx-site-v19 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for ngrx-io canceled.Built without sensitive environment variables
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration looks good.
Can you look at the lint errors please to make the build pass?
@jdegand there seem to be some linting issues. |
@timdeschryver I think some issues stem from this in the config files: parserOptions: {
project: ['modules/effects/testing/tsconfig.*?.json'],
}, There are issues resolving file paths. I going to try tweaking this first. Changing the wildcard helped 3 more lints pass. |
Using
for the |
There seem to be inconsistencies between the results of CI tests and what I get in a GitHub codespace. I am getting more lint successes in a GitHub codespace. If high CPU usage happens in the codespace, results seem to become unreliable. |
Now, I think the question mark after the wildcard didn't make a difference ( Running I changed the two e2e configs so the |
|
modules/store/tsconfig.build.json
Outdated
"paths": { | ||
"@ngrx/store": ["src"] | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this change required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was part of my debugging (this is just helping to point to the correct files to lint) and it's not necessary. Looks like the only file I did this in.
@@ -92,7 +92,8 @@ | |||
"!{projectRoot}/tsconfig.spec.json", | |||
"!{projectRoot}/jest.config.[jt]s", | |||
"!{projectRoot}/.eslintrc.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems this line can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there is a leftover eslintrc.json
file in the projects/www
folder. Because it uses analog, I guess the nx script skipped converting it automatically. I guess this can be converted as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had some issues getting this working perfectly, but it looks there will be extra lint errors that need to be fixed inside the www
folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new style guide generated quite a few errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if you want this included into this PR or a separate PR. I created another branch to work on and implemented fixes there. I can easily do whatever you prefer. Although it does clutter the PR, I did remove standalone:true
from a lot of components in the www
folder (might as well do it now).
return PropsFilterFnFactory<Villain>[('name', 'saying')]( | ||
entities, | ||
pattern | ||
return ( | ||
PropsFilterFnFactory < | ||
Villain > | ||
[('name', 'saying')](entities, pattern) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you revert this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used no-verify
to revert the file.
|
||
export default [ | ||
{ | ||
ignores: ['**/dist', '**/jest.config.ts', '**/schematics-core/test-setup.ts', '**/schematics-core/utility/standalone.ts'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add this to the base config and remove it in the module specific configs?
In the base config:
ignores: ['**/dist', '**/jest.config.ts', '**/schematics-core'],
I don't remember why we excluded these, this can be looked into later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the exclusions were added to match previous files. The lints fail without them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential option is to add .eslintignore
files and add the files there.
In base config, could add an overrides array like this:
overrides: [
{
files: ['**/jest.config.ts'],
env: { node: true },
rules: {
// customize rules for config files
},
},
{
files: ['**/schematics-core/**/*.ts'],
env: { node: true },
rules: {
// loosen rules or add globals as needed
},
},
],
Typically, files are ignored if they use "globals" or unusual patterns that could break if linted. Code that uses TypeScript AST, Node.js (node vs web environments), and schematics is also typically ignored. That standalone utility looks a stop-gap or bridge file and may be something that could potentially be removed as bootstrapApplication
can make standalone components.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Change to ESLint flat config.
What is the current behavior?
Outdated ESLint configuration. Failing lint command in
package.json
.Closes #4883
What is the new behavior?
ESLint works with
lint
command.Does this PR introduce a breaking change?
Other information
There are quite a few lint tests failing.