eslint warn floating promises

[turadg]

closes: #5407

Description

Enable no-floating-promises. This defers solving all the floating promises by using warn level instead of error. Eventually we'll want to error but I propose we get there incrementally, distributed among teams.

This rule from @typescript-eslint is the first we've enabled to require type information (by setting parserOptions.project) . That increases the time cost of linting. Measured with time yarn lerna run --no-bail lint:eslint on my machine:

• before: 84.86s user 5.35s system 454% cpu 19.827 total (1.4min)
• after: 824.27s user 135.32s system 556% cpu 2:52.39 total (14min)

In CI the lint job went from 5 min before to 21 min after. To prevent developers having to wait longer for PRs to complete, this splits the lint job into two lint-primary and lint-rest.

The comments in that job config note that performance could be greatly improved if the linter were able to reuse type information across projects. Alternately we could unify the packages into one TypeScript project.

I also tried cutting the lint job time by shortening the setup step, replacing restore-node with setup-node and yarn install from cache. This cut 5min setup time down to 2min. (Saving 6min compute over the two jobs.) But it failed because there are build artifacts the linting depends on, such as:

• eslint import/no-unresolved fails when @agoric/ui-components isn't built
• typescript cannot find module '../bundles/bundle-contractFacet.js' in ./zoe/tools/fakeVatAdmin.js (maybe this one should use bundleCache instead?)

Security Considerations

--

Documentation Considerations

Notify developers that they'll begin seeing new warnings and that the Github PR annotations may show many more in the code review web UI.

Testing Considerations

Verified that lint jobs are not the slowest zebra: iIn last run lint-primary (10m) and lint-rest (12m) were both less than test-cosmic-swingset (13m) and test-swingset3 (13min).

[michaelfig]

Great stuff! Thanks for shepherding this through.

[mhofman]

Sorry I haven't had time to look at this yet. I'd like to review before this lands.

[mhofman]

Running yarn lint locally took 33 minutes and failed with an OOM in the solo package.

Let's find a way to reduce the impact of this change.

[turadg]

Running yarn lint locally took 33 minutes and failed with an OOM in the solo package.

Recording numbers from out chat: (in packages/zoe)
mhofman's VM: 99s before, 1221s after (~12x slowdown)
turadg's M1: 5s before, 42s after (~8x slowdown)

Let's find a way to reduce the impact of this change.

👍 e1625fe makes it opt in with an AGORICSDK_ESLINT_ENABLE_TYPE_RULES flag. CI will use this and those who want to use it locally can enable it too.

[mhofman]

Thanks for making this optional. One change request for the env check.

Should we also add a new script to the root package.json that runs lint:packages with the AGORICSDK_ESLINT_ENABLE_TYPE_RULES="true" and NODE_OPTIONS="--max-old-space-size=8192" set (which apparently is needed on my machine, not just in CI). We can bike-shed the name, something like lint:with-types ?

[mhofman]

I'd prefer an explicit test for === '1' || === 'true' as the above would be truthy with AGORICSDK_ESLINT_ENABLE_TYPE_RULES = '0'

[turadg]

I hear you would prefer that and you put a block on merge because of it. Imo "I'd prefer" is not a reason to block a change.

I could add an explicit test but I don't think it's worth it given how this will be used. It's reasonable for an env flag to be based on presence. Knowing the people who are going to use this flag, I don't expect anyone will try AGORICSDK_ESLINT_ENABLE_TYPE_RULES=0.

I'm not claiming it won't be better if it did handle that, just that it's not worth the added code or time to write it or blocking a PR.

[michaelfig]

I would prefer to use the presence of $CI to determine the linting mode. Totally easy to set: CI=t yarn lint.

[turadg]

wfm. I considered CI but felt aversion because I wanted people to run it locally too, but pros:

1. it is a much more conventional flag
2. it often means "do more, slower stuff"
3. makes obvious when using it that it's the same settings as run in CI

[mhofman]

CI is really a conflation. I'm not really a fan of implicit behavior changes based on environment sniffing. As @turadg mentions, there is value in being able to explicitly run this test locally.

[michaelfig]

I think this, combined with your other comment, is the best argument for a long-named environment variable (but please, AGORIC_ prefix rather than SMASHINGTHE_FIRST_TWO_WORDS_TOGETHER), plus a script that runs with the appropriate NODE_OPTIONS and enabling environment variable.

Then CI can use that script directly as well. No need to put it in package.json, I think, unless it's something like lint-slowly :)

[turadg]

Should we also add a new script to the root package.json that runs lint:packages

All the lint:* packages get run by yarn lint so that would duplicate. We could have a name in the root doesn't start with lint: but in package.json we'd lose the ability to document why the env vars are necessary. We could have a separate script, but I find the existing solution sufficient. It works for CI. On my machine I'll set the env var. On yours you know what to do. On others they can follow either example.

If this were production code the standards would be higher but it's just a little option for stronger linting. I expect our linting to get refactored more soon so don't want to create more commands that have to keep working as expected. This env var has the air of something that could go away.

[mhofman]

All the lint:* packages get run by yarn lint so that would duplicate.

Right, I forgot about that.

On yours you know what to do.

No I don't. Me 3 days ago learned that I also need to set NODE_OPTIONS or I fail with OOM. Me in a few days will have forgotten. Calling commands with explicit env is fairly unergonomic if these are not documented anywhere (I guess they're documented in the GitHub actions yml files, but that's kinda obscure and not where I'd look in the first place).

I suppose I would have less qualms if somehow the .eslintrc.js file would somehow automatically set the required max-old-space-size if the env to trigger type checks is set.