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

Update deps; New eslint config; Fix budgeted fs wrapper bug #1185

Merged
merged 7 commits into from
Sep 11, 2024
Merged

Update deps; New eslint config; Fix budgeted fs wrapper bug #1185

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ef2b05e
Update dependencies requiring no changes
aomarks Sep 7, 2024
9ad8990
Upgrade eslint and convert to new config file format
aomarks Sep 7, 2024
66425d8
Fix style things
aomarks Sep 7, 2024
7369c40
Fix bug with budgeted fs wrapper
aomarks Sep 7, 2024
d356d98
Don't use tseslint.config
aomarks Sep 8, 2024
ecfb2a1
Update eslint config
aomarks Sep 8, 2024
4d78d94
Simpler
aomarks Sep 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions .eslintignore
View file
Open in desktop

This file was deleted.

25 changes: 0 additions & 25 deletions .eslintrc.cjs
View file
Open in desktop

This file was deleted.

7 changes: 6 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic
Versioning](https://semver.org/spec/v2.0.0.html).

<!-- ## Unreleased -->
## Unreleased

### Fixed

- Fix a bug that may have resulted in Wireit attempting to open too many files
at once (no known reports).

## [0.14.9] - 2024-09-03

Expand Down
59 changes: 59 additions & 0 deletions eslint.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/**
* @license
* Copyright 2024 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import pluginJs from '@eslint/js';
import noOnlyTests from 'eslint-plugin-no-only-tests';
import tseslint from 'typescript-eslint';

export default tseslint.config(
{
ignores: [
'**/*.js',
Copy link
Collaborator

@justinfagnani justinfagnani Sep 7, 2024 •
edited by aomarks
Loading

Choose a reason for hiding this comment

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

I'm surprised that tseslint isn't already ignoring files not in the tsconfig. Is there a way to do that?

Otherwise, might this help? https://github.com/antfu/eslint-config-flat-gitignore

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you use a path argument when you run eslint in your npm script, like eslint src/**/*.ts or whatever?

I like to set eslint up so that you can run it with no path argument. My experience is that when you run eslint with no path argument, it will try and lint all supported files. The tseslint config may only apply to typescript project files, but eslint is still going to try and lint everything else with the non-tseslint rules.

So I think it's still good to have all your non-lintable files listed here.

Syncing with .gitignore makes sense, but I think I'm going skip that plugin since the structure changes so rarely and I don't want to add a dep. I did clean up the list, though.

'**/*.cjs',
'**/*.mjs',
'**/*.d.ts',
'**/.wireit/',
'**/node_modules/',
'lib/',
'vscode-extension/.vscode-test/',
'vscode-extension/lib/',
'vscode-extension/built/',
],
},
{
files: ['**/*.ts'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be src/**/*.ts?

Copy link
Member Author

@aomarks aomarks Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think **/*.ts is actually fine, because if we're globally ignoring all non-source files, then a general ".ts extension" filter seems ok.

This way we don't have to know anything about the project layout, which in this case would also require that we add vscode-extension/src.

languageOptions: {
parser: tseslint.parser,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't think you had to set the parser manually anymore.

Here's a recent config I made when upgrading to ESLint v9:

import tseslint from 'typescript-eslint';
import eslint from '@eslint/js';
import eslintPluginPrettierRecommended from 'eslint-plugin-prettier/recommended';

export default [
  eslint.configs.recommended,
  eslintPluginPrettierRecommended,
  ...tseslint.configs.recommendedTypeChecked,
  {
    languageOptions: {
      parserOptions: {
        projectService: true,
        tsconfigRootDir: import.meta.dirname,
      },
    },
    rules: {
      "@typescript-eslint/no-non-null-assertion": "error"
    },
  },
];

It doesn't use tseslint.config() because the config is in .js and doesn't need a type-safe config merger.

Copy link
Member Author

@aomarks aomarks Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't think you had to set the parser manually anymore.

Huh, you're right! I think I was getting mislead by a combination of things:

  • My config was initially matching .js files not covered by a TypeScript config because I didn't have the right files filter.

  • eslint then tried to run a TypeScript-parser-depending rule on that js file, but that rule has an outdated error message saying that you need to set a parser. But the real problem was just that it shouldn't have been covered by the TypeScript rule at all.

It doesn't use tseslint.config() because the config is in .js and doesn't need a type-safe config merger.

Ah nice. Didn't realize this was just a typings thing.

Here's a recent config I made when upgrading to ESLint v9:

Cool! I adopted some of this.

The main difference now is that I have this {files: ["**/*.ts"]} constraint on all the TypeScript setting objects (including the tseslint ones). What I found is that I otherwise get errors with projectService: true where tseslint is not able to find a tsconfig.json for the one or two .js files we have here. A little odd, but I can't find a simpler arrangement after fiddling for a while -- I always end up with some kind of error relating to non-ts files.

Copy link
Member Author

Choose a reason for hiding this comment

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

eslintPluginPrettierRecommended,

Oh also, curious why you like using this prettier eslint plugin? (instead of just using the prettier binary).

parserOptions: {
project: ['./tsconfig.json', './vscode-extension/tsconfig.json'],
},
},
plugins: {
'@typescript-eslint': tseslint.plugin,
'no-only-tests': noOnlyTests,
},
},
pluginJs.configs.recommended,
...tseslint.configs.strictTypeChecked,
{
rules: {
'no-only-tests/no-only-tests': 'error',
'@typescript-eslint/no-unused-vars': [
'error',
{argsIgnorePattern: '^_', varsIgnorePattern: '^_'},
],
'@typescript-eslint/no-non-null-assertion': 'off',
'@typescript-eslint/no-useless-constructor': 'off',
'@typescript-eslint/only-throw-error': 'off',
'@typescript-eslint/no-confusing-void-expression': 'off',
'@typescript-eslint/restrict-template-expressions': 'off',
'@typescript-eslint/no-unnecessary-condition': 'off',
'@typescript-eslint/no-unnecessary-type-arguments': 'off',
'@typescript-eslint/no-unnecessary-template-expression': 'off',
'@typescript-eslint/use-unknown-in-catch-callback-variable': 'off',
},
},
);
Loading