Upgrade to ESLint v9 #10846

iomekam · 2025-01-14T09:50:40Z

Description

Updates ESLint to v9. Doing so deprecates the use of .eslintrc and eslintignore and moves all these configuration options into a flat config file, eslint.config.mjs. As part of this change, we also needed to update the github eslint plugin as well as the jessie.js eslint plugin. Most of the rules were migrated successfully, but I had to modify a few rules since ESLint v9 comes with a few default behavior changes.

Security Considerations

N/A

Scaling Considerations

N/A

Documentation Considerations

Here's a breakdown of the breaking changes that come with ESLint v9. https://eslint.org/docs/latest/use/migrate-to-9.0.0#-varsignorepattern-option-of-no-unused-vars-no-longer-applies-to-catch-arguments.

Testing Considerations

I modified our yarn lint command so I could individually run and view the error of each package. Doing so allowed me to debug every single error that occured after the ugprade and make changes to restore expected behavior.

Upgrade Considerations

N/A

cloudflare-workers-and-pages · 2025-01-14T09:54:36Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`6bd3574`
Status:	✅ Deploy successful!
Preview URL:	https://eae755c1.agoric-sdk.pages.dev
Branch Preview URL:	https://iomekam-es-lint-2.agoric-sdk.pages.dev

View logs

turadg · 2025-01-15T22:14:52Z

eslint.config.mjs

+        "packages/cosmic-proto/node_modules/",
+        "packages/cosmic-proto/coverage/",
+        "packages/cosmic-proto/dist/",


are these not implied by the simpler globs? we want to ignore these no matter where they are:

coverage

dist

node_modules

These were migrated from all our our current eslintignore files. I do think it makes sense to have these ignored gobally though

turadg · 2025-01-15T22:15:50Z

eslint.config.mjs

+
+export default [{
+    ignores: [
+        "coverage/**/*",


does .eslintignore no longer work?

Yeah it's deprecated

eslint.config.mjs

turadg · 2025-01-15T22:16:42Z

eslint.config.mjs

+
+        "ava/no-skip-test": "off",
+        "ava/use-test": "off",
+        "@jessie.js/safe-await-separator": "off", // Temporarily disabled due to ESLint 9.x compatibility issue


can be re-enabled?

Thanks for catching this

eslint.config.mjs

turadg · 2025-01-15T22:18:49Z

packages/cosmic-swingset/tsconfig.json

@@ -1,9 +1,12 @@
 // This file can contain .js-specific Typescript compiler config.
 {
  "extends": "../../tsconfig.json",
+  "compilerOptions": {},


what does this do?

Not needed. I was testing something out and forget to remove this

turadg · 2025-01-15T22:19:12Z

packages/cosmic-swingset/types.d.ts

+import { SpawnSyncReturns as NodeSpawnSyncReturns } from 'child_process';
+
+declare module 'child_process' {
+  export interface SpawnSyncReturns<T> extends NodeSpawnSyncReturns<T> {


why was this necessary?

We were previously ignoring packages/cosmic-swingset/check-validator.cjs when using our old config. Given that it had check-jessie, so I assume it was meant to be linted. When trying to lint, I got this error:

check-validator.cjs:25:9 - error TS2339: Property 'code' does not exist on type 'SpawnSyncReturns<Buffer<ArrayBufferLike>>'. 25 if (ret.code) { ~~~~ check-validator.cjs:26:20 - error TS2339: Property 'code' does not exist on type 'SpawnSyncReturns<Buffer<ArrayBufferLike>>'. 26 process.exit(ret.code);

I added this to get the linting working. I'm open to alternatives

I'm pretty sure it's a bug. Node docs for spawnSync and the TS defs show a status: number property which I think is what this meant to use.

Nice catch!

turadg · 2025-01-17T15:20:03Z

eslint.config.mjs

+const allDeprecated = [...deprecatedForLoanContract, ['loan', 'debt']];
+
+const deprecatedTerminology = Object.fromEntries(
+Object.entries({


This isn't formatted correctly. Please make sure this file is included in the format and lint:format selections

turadg · 2025-01-17T15:21:37Z

eslint.config.mjs

+
+        "ava/no-skip-test": "off",
+        "ava/use-test": "off",
+        "@jessie.js/safe-await-separator": "error", // Temporarily disabled due to ESLint 9.x compatibility issue


comment no longer applicable

turadg · 2025-01-17T15:22:22Z

eslint.config.mjs

+    // Include both .js and .ts files for cache package
+    files: ["packages/cache/**/*.{js,ts,mjs,cjs}"],


shouldn't this be the glob for all packages?

I don't think we were previously including *.ts files.

casting and cache both explicilty ran eslint --ext .js,.ts .

turadg

almost there.

before I forget to say: before merging the commits will need cleaning up. there aren't any actual fix commits here for the changelog. Just one that updates ESlint and another that fixes the .code bug

turadg · 2025-01-17T19:41:21Z

eslint.config.mjs

+      'packages/cosmic-proto/src/codegen/',
+      'packages/cosmic-proto/scripts/',
+      // Cosmic-swingset specific ignores
+      'packages/cosmic-swingset/src/bundles/**/*',


this should be a global ignore for bundles at any path

turadg · 2025-01-17T19:41:30Z

eslint.config.mjs

+      'packages/cosmic-swingset/src/bundles/**/*',
+      'packages/cosmic-swingset/t[0-9]/**/*',
+      'packages/cosmic-swingset/t[0-9].*/**/*',
+      'packages/cosmic-swingset/node_modules/**/*',


ignored above

turadg · 2025-01-17T19:41:43Z

eslint.config.mjs

+      'packages/cosmic-swingset/t[0-9]/**/*',
+      'packages/cosmic-swingset/t[0-9].*/**/*',
+      'packages/cosmic-swingset/node_modules/**/*',
+      'packages/cosmic-swingset/build/**/*',


this should be another global ignore

turadg · 2025-01-17T19:42:50Z

eslint.config.mjs

+export default [
+  {
+    ignores: [
+      '**/coverage/**/*',


all these directory ignores don't need the trailing glob, right?
https://eslint.org/docs/latest/use/configure/ignore#ignoring-directories

turadg

🎉

turadg · 2025-01-18T15:02:05Z

multichain-testing/scripts/fast-usdc-tool.ts

@@ -205,7 +205,6 @@ const main = async () => {
        instancePath: [contractName],
        callPipe: [['makeDepositInvitation']],
      },
-      // @ts-expect-error 'NatAmount' vs 'AnyAmount'


Why is this eslint change affecting TypeScript?

It may be because of some changes to tsconfig, but I couldn't figure out how to keep this line without this error:

scripts/fast-usdc-tool.ts:208:7 - error TS2578: Unused '@ts-expect-error' directive. 208 // @ts-expect-error 'NatAmount' vs 'AnyAmount' ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

turadg · 2025-01-18T15:03:53Z

multichain-testing/scripts/make-bank-asset-info.ts

@@ -3,7 +3,7 @@

 import '@endo/init';
 import starshipChainInfo from '../starship-chain-info.js';
-import { makeAssetInfo } from '../tools/asset-info.ts';


This is a code change, not just lint change. I suspect it will break this test. I've added "force:integration" to check

turadg · 2025-01-18T15:08:34Z

eslint.config.mjs

+      'import/no-extraneous-dependencies': 'off',
+    },
+  },
+  // Separate config for multichain-testing that doesn't inherit root rules


It's a separate Yarn project so I think we'd be better off giving it a desperate flag confit.

Ie leave its dependencies and source alone. If the eslint config for the root project is affecting multichain-testing, fix only that. (Eg by putting a config file in there so it will get that one instead)

Regardless we need an issue to come and make this other yarn project in the repo lint correctly. Something like #10238

They don't have a supported way of doing this. I did some digging and they have an experimental approach to this. We can leverage this as long as we understand it's not a fully released feature yet

That link says the default is to start search from the CWD. Why wouldn't an eslintconfig within multichain-testing already resolve before the one in agoric-sdk?

Just tested and this does indeed work without the flag

turadg · 2025-01-18T15:09:30Z

eslint.config.mjs

+        },
+      ],
+      // Disable all other rules
+      '@typescript-eslint/no-floating-promises': 'off',


I want to see these suppressions omitted but if we do end up keeping them please use "warn" instead of disabling them completely. They should still give notice to developers even if they don't fail CI.

turadg · 2025-01-21T21:22:55Z

multichain-testing/package.json

@@ -91,29 +91,5 @@
    "@agoric/zoe": "portal:../packages/zoe",
    "@agoric/zone": "portal:../packages/zone"
  },
-  "eslintConfig": {


please separate multichain-testing changes to another PR. It's complicating this migration of agoric-sdk root project. Let's get that landed.

Without these changes, linting will fail for multichain-testing as it'll pick up the root config and try to use that

Hmm, I just tried it and it seemed to work.

cd multichain-testing git checkout master . yarn install yarn lint

Do you get a different result?

btw, assuming we can punt updating multichain-testing, this PR should no longer be marked as closing 10642

This does work locally. I'll remove the changes and observe CI. It was picking up the new config in CI, but maybe I just observed the wrong thing and chased the wrong path

Failing per here. Based on these lines, it should be emulating what we tried locally

Going back to this comment, when you ran git checkout master ., I don't think it removed the eslint.config.mjs file that was newly added as part of this change, so when you ran yarn lint, it was still using that file

Very confusing when CI has a different result for yarn lint than the local checkout does. I tried diagnosing why the linked job failed but I couldn't determine what state of this branch it was using and what changes it had to multichain-testing.

So I made a new branch, https://github.com/Agoric/agoric-sdk/tree/ta/eslint-v9, which has the steps I pasted above. It's passing in https://github.com/Agoric/agoric-sdk/actions/runs/12898290669/job/35965066953

Maybe the failing job did not have the master state of multichain-testing?

Even when you reverted the change in the eslint-v9 branch, the new eslint.config.mjs file is still present in the latest version of the branch

Ah. My branch doesn't actually match what I was asking for. And upon closer inspection it looks like it's green because in master the linter is broken in this dir.

So thanks for fixing it!

turadg

One small change to make. Before merge please rebase locally to fix the "fixup! fixup!" which automerge won't know how to handle.

turadg · 2025-01-22T16:37:06Z

multichain-testing/package.json

@@ -5,7 +5,7 @@
  "scripts": {
    "build": "exit 0",
    "lint": "yarn tsc && yarn eslint .",
-    "lint-fix": "yarn lint:eslint --fix",
+    "lint-fix": "yarn lint --fix",


yarn lint just happens to end in eslint. Since we don't have lint: scripts in this package.json this one should call that directly.

Suggested change

"lint-fix": "yarn lint --fix",

"lint-fix": "yarn eslint . --fix",

turadg · 2025-01-22T16:44:28Z

multichain-testing/package.json

@@ -91,29 +91,5 @@
    "@agoric/zoe": "portal:../packages/zoe",
    "@agoric/zone": "portal:../packages/zone"
  },
-  "eslintConfig": {


Ah. My branch doesn't actually match what I was asking for. And upon closer inspection it looks like it's green because in master the linter is broken in this dir.

So thanks for fixing it!

mergify · 2025-01-23T05:36:18Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

❌ integration-test-result
☑️ linear-history

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

iomekam · 2025-01-23T05:53:54Z

@Mergifyio requeue

mergify · 2025-01-23T05:53:57Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

iomekam force-pushed the iomekam-es-lint-2 branch from ac1e669 to 615e12d Compare January 14, 2025 14:36

iomekam changed the title ~~Iomekam es lint 2~~ Upgrade to ESLint v9 Jan 15, 2025

iomekam force-pushed the iomekam-es-lint-2 branch 3 times, most recently from 7775513 to 3d589b1 Compare January 15, 2025 21:17

iomekam marked this pull request as ready for review January 15, 2025 21:30

iomekam requested a review from a team as a code owner January 15, 2025 21:30

iomekam requested review from AgoricTriage and turadg January 15, 2025 21:30

turadg reviewed Jan 15, 2025

View reviewed changes

turadg reviewed Jan 17, 2025

View reviewed changes

iomekam force-pushed the iomekam-es-lint-2 branch from 866b354 to 8bd868d Compare January 17, 2025 23:02

turadg approved these changes Jan 17, 2025

View reviewed changes

iomekam added the automerge:rebase Automatically rebase updates, then merge label Jan 18, 2025

iomekam force-pushed the iomekam-es-lint-2 branch from 633ca30 to 8f15007 Compare January 18, 2025 01:29

iomekam removed the automerge:rebase Automatically rebase updates, then merge label Jan 18, 2025

turadg added the force:integration Force integration tests to run on PR label Jan 18, 2025

turadg requested changes Jan 18, 2025

View reviewed changes

iomekam force-pushed the iomekam-es-lint-2 branch 2 times, most recently from 9b673b2 to 675172a Compare January 21, 2025 17:19

Agoric deleted a comment from mergify bot Jan 21, 2025

iomekam requested a review from turadg January 21, 2025 19:46

turadg reviewed Jan 21, 2025

View reviewed changes

iomekam force-pushed the iomekam-es-lint-2 branch from 8d2bdd3 to 2fa6631 Compare January 21, 2025 22:23

turadg approved these changes Jan 22, 2025

View reviewed changes

iomekam force-pushed the iomekam-es-lint-2 branch from 8d2bdd3 to 6593986 Compare January 22, 2025 18:34

iomekam added the automerge:rebase Automatically rebase updates, then merge label Jan 23, 2025

chore(eslint): upgrade to v9

f9186c9

iomekam added 2 commits January 23, 2025 04:43

chore(cosmic-swingset): fix linting by using status instead of code

78b053b

chore(multichain-testing): adapt to eslint v9 flat config

6bd3574

iomekam force-pushed the iomekam-es-lint-2 branch from 6593986 to 6bd3574 Compare January 23, 2025 04:43

mergify bot merged commit c9a663b into master Jan 23, 2025
83 checks passed

mergify bot deleted the iomekam-es-lint-2 branch January 23, 2025 06:12

turadg mentioned this pull request Jan 23, 2025

upgrade ESlint to v9 endojs/endo#2682

Open

		// Include both .js and .ts files for cache package
		files: ["packages/cache/*/.{js,ts,mjs,cjs}"],

	"lint-fix": "yarn lint --fix",
	"lint-fix": "yarn eslint . --fix",

Upgrade to ESLint v9 #10846

Upgrade to ESLint v9 #10846

Conversation

iomekam commented Jan 14, 2025 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Jan 14, 2025 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

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

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

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

Choose a reason for hiding this comment

turadg 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

turadg 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

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

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

iomekam Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

iomekam Jan 21, 2025 • edited Loading

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

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Jan 23, 2025

iomekam commented Jan 23, 2025

mergify bot commented Jan 23, 2025

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

iomekam commented Jan 14, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Jan 14, 2025 •

edited

Loading

iomekam Jan 21, 2025 •

edited

Loading

iomekam Jan 21, 2025 •

edited

Loading