build(oxlint): upgrade oxlint to 1.75 and tsgolint to TS 7 - #22561
Conversation
logaretm
changed the title
size-limit report 📦
|
02b93b1 to
af518ac
Compare
logaretm
changed the title
af518ac to
977d8d4
Compare
977d8d4 to
d5565e9
Compare
| // If we have no comparison branch, we just run size limit & store the result as artifact | ||
| if (!comparisonBranch) { | ||
| return runSizeLimitOnComparisonBranch(); | ||
| return await runSizeLimitOnComparisonBranch(); |
There was a problem hiding this comment.
Example of the new return await rule detecting returning promises inside try/catch blocks.
d38291a to
3f6b2e1
Compare
e4e78c5 to
5efdd5d
Compare
s1gr1d
left a comment
s1gr1d
left a comment
There was a problem hiding this comment.
I see a lot of removed type casts - is this because the type can be better inferred now or the linting does not allow the casts anymore?
Would be good to still be able to cast when necessary, also to better see which type we would expect, when it's not inferred (however, we can always opt-out with a disable comment)
| const exceptionValue: Exception = { | ||
| type: isEvent(exception) ? exception.constructor.name : isUnhandledRejection ? 'UnhandledRejection' : 'Error', | ||
| value: getNonErrorObjectExceptionValue(exception, { isUnhandledRejection }), | ||
| }; | ||
|
|
||
| const event = { | ||
| exception: { | ||
| values: [ | ||
| { | ||
| type: isEvent(exception) ? exception.constructor.name : isUnhandledRejection ? 'UnhandledRejection' : 'Error', | ||
| value: getNonErrorObjectExceptionValue(exception, { isUnhandledRejection }), | ||
| } as Exception, | ||
| ], | ||
| values: [exceptionValue], |
There was a problem hiding this comment.
Q: which rule is that? Or was this just a small refactor?
There was a problem hiding this comment.
no-unnecessary-type-assertion, I think it was always there but never worked due to the diagnostic suppression we had in place to get it to work with TS 5
|
@s1gr1d Basically it is able to infer types better now and is able to determine if a type is unnecessary. Rule def can be found here.
Yep we still are able to, it doesn't ban casts, it bans the ones it deems unnecessary. In all of these cases we already had the right types inferred. I checked most of them. I think it's probably due to our codebase drifting around or us tightening the types in unrelated work. Note there are indeed a few false positives but around 7% of the cases here, so I think its worthwhile. |
Renames/drops the rules the new versions flag as unknown: `no-return-await` becomes `typescript/return-await` (kept on the modern in-try-catch default), and rules removed upstream are dropped since they no longer exist natively: `quotes` (now owned by oxfmt), `import/no-unresolved` (was off everywhere), `import/no-extraneous-dependencies`, `jsdoc/require-jsdoc`, and `react/prop-types`. Also removes the `OXLINT_TSGOLINT_DANGEROUSLY_SUPPRESS_PROGRAM_DIAGNOSTICS` workaround from every lint script so tsgolint surfaces real program diagnostics again.
Clears the type-aware errors that appeared once real program diagnostics were re-enabled: - Removes unnecessary type assertions and the now-unused type imports they leave behind (oxlint --fix), and drops a dead pino integration interface. - Consolidates the dedupe stacktrace/fingerprint guards so narrowing works without the old assignment-cast idiom. - Migrates the terser plugin from the deprecated `output` option to `format`. - Renames stale disable directives to the new rule names (`typescript-eslint(...)` -> `typescript/...`) for unbound-method and the HTTP_URL no-deprecated suppressions. - Adds no-deprecated suppressions where we intentionally read our own deprecated options for back-compat, and return-await suppressions where an await is deliberate (node-cron) or the cast makes the rule misfire.
…ssary `no-unnecessary-type-assertion` (tsgolint) reports several casts as redundant, but they are load-bearing: it only checks assignability at the cast site and misses that the cast changes the expression's resulting type for downstream use. Removing them type-checks locally but fails the real `tsc` build (verified: both tsconfig.json and tsconfig.types.json error identically without the cast). Restores each cast with a targeted suppression: a Cloudflare-only client option, LCP/INP PerformanceEntry fields, a Vue VNode probe, mysql/postgres vendored connection shapes, the view-hierarchy identifier, and the internal `shouldNotThrowOnFailure` on setCommits. eventbuilder instead uses a typed `Exception` variable so no cast (or suppression) is needed there.
The oxlint 1.75 bump surfaced ~5.9k warnings. This clears them to zero without churning the codebase: - Vitest: keep the plugin (and its ~60 other rules) but disable the correctness rules the bump newly fired across the existing suite (no-standalone-expect, require-mock-type-parameters, no-conditional-expect, expect-expect, valid-title, valid-expect, require-to-throw-message, no-disabled-tests, valid-describe-callback). - Disable rules that over-trigger / false-positive here: no-useless-default-assignment, no-useless-fallback-in-spread, no-thenable (we implement thenables), no-new-array (used with immediate .fill()), prefer-string-starts-ends-with. - Keep no-empty-file on (catches accidental empties) but exempt the two intentional comment-only files, and silence test-file occurrences of the general rules. - Fix the few worth fixing: drop redundant `void` operators in the graphql/firebase channel callbacks, type the bun test-runner fetch init as RequestInit, and suppress the two no-unreachable false positives in rollup dual-build helpers. Note: vitest/valid-expect flagged 10 pre-existing broken tests (chai-style `.to.` modifiers); left for a separate fix.
…base
The develop rebase reintroduced the OTel span casts (its span-kind refactor
touched this file); tsgolint flags them as unnecessary again, so drop them
while keeping develop's `attributes ... || {}` change.
waitForReplayEventBuffer hardcoded "one Promise.resolve() per await in the util functions", so it silently coupled the test to the enrichment chain's exact await count. That made a behavior-neutral lint fix (dropping a redundant `return await` in _getResponseText) look like a data-loss regression. Flush microtasks in a bounded loop instead, so the test no longer depends on the implementation's await depth. _getResponseText keeps the rule-compliant `return response.text()`.
Astro's middleware no longer needs the `next` cast (unnecessary post-rebase), and the new Durable Object storage allowlist cast gets the same targeted `no-unnecessary-type-assertion` suppression as its SQL sibling, since the Cloudflare-only option is invisible to the rule but required by tsc.
5efdd5d to
e0447c5
Compare
I've noticed that LLMs are often very liberal in putting typecasts everywhere. It is often the safer option and simplest way to avoid a type conflict, which I guess is why they do it, but it has led to a lot of unnecessary casts (which then sometimes Bugbot complains about in the PR), so having a linter that removes these is nice. 👍 |
| }, | ||
| }, | ||
| output: { | ||
| format: { |
There was a problem hiding this comment.
deprecated API, was being flagged by the linter which I assume due to a direct result to the type awareness being "fully" aware now.
| }, | ||
| "scripts": { | ||
| "lint:fix": "OXLINT_TSGOLINT_DANGEROUSLY_SUPPRESS_PROGRAM_DIAGNOSTICS=true oxlint . --fix --type-aware", | ||
| "lint": "OXLINT_TSGOLINT_DANGEROUSLY_SUPPRESS_PROGRAM_DIAGNOSTICS=true oxlint . --type-aware", |
There was a problem hiding this comment.
So nice to drop this giant env flag 🏁
| // _safeRead falls back to direct property access. | ||
| } | ||
| // oxlint-enable typescript-eslint(unbound-method) | ||
| // oxlint-enable typescript/unbound-method |
There was a problem hiding this comment.
Ah, oxlint finally aligning with every other linter's way of identifying rules. <3 <3 <3
There was a problem hiding this comment.
this was so annoying, yes!
| { name: 'glob', scope: sentryScope }, | ||
| async () => await globFiles(globAssets, { ignore: options.sourcemaps?.ignore }), | ||
| const globResult = await startSpan({ name: 'glob', scope: sentryScope }, async () => | ||
| globFiles(globAssets, { ignore: options.sourcemaps?.ignore }), |
There was a problem hiding this comment.
This return-await rule is quite nice. I like it.
| // oxlint-disable-next-line typescript/no-unnecessary-type-assertion -- rule false positive: the cast carries the internal `shouldNotThrowOnFailure` field; tsc errors without it | ||
| setCommits: userOptions.release?.setCommits as |
There was a problem hiding this comment.
I'm pretty sure you could do this without the override by doing something like this up above:
const setCommits:
| (SetCommitsOptions & { shouldNotThrowOnFailure?: boolean })
| false
| undefined = userOptions.release?.setCommits;and then replacing this line with:
setCommits,But, the override is fine, too. Maybe better, since otherwise it'd move the casting assignment way out of view.
There was a problem hiding this comment.
it looks like it doesn't take the type narrowing into account, it checks if the operand is assignable already to the cast and if it is then it flags it.
There are a few filed bugs with it but I think if the type cast is a superset of the inferred type, it should not flag it. Well, ideally that is.
| // oxlint-disable-next-line typescript/no-unnecessary-type-assertion -- rule false positive: the cast reaches the Cloudflare-only `durableObjectStorageSpanAllowlist`; tsc errors without it | ||
| const allowlist = (getClient()?.getOptions() as CloudflareClientOptions | undefined) |
There was a problem hiding this comment.
Would this work?
| // oxlint-disable-next-line typescript/no-unnecessary-type-assertion -- rule false positive: the cast reaches the Cloudflare-only `durableObjectStorageSpanAllowlist`; tsc errors without it | |
| const allowlist = (getClient()?.getOptions() as CloudflareClientOptions | undefined) | |
| const allowlist: CloudflareClientOptions | undefined = getClient()?.getOptions() |
There was a problem hiding this comment.
Could work if I extract the options to its own variable up there. I ignored for now since I think this is a false positive.
| // oxlint-disable-next-line typescript/no-unnecessary-type-assertion -- rule false positive: the cast reaches the Cloudflare-only `durableObjectSqlSpanAllowlist`; tsc errors without it | ||
| const allowlist = (getClient()?.getOptions() as CloudflareClientOptions | undefined) | ||
| ?.durableObjectSqlSpanAllowlist; |
There was a problem hiding this comment.
Same here, maybe?
| // oxlint-disable-next-line typescript/no-unnecessary-type-assertion -- rule false positive: the cast reaches the Cloudflare-only `durableObjectSqlSpanAllowlist`; tsc errors without it | |
| const allowlist = (getClient()?.getOptions() as CloudflareClientOptions | undefined) | |
| ?.durableObjectSqlSpanAllowlist; | |
| const allowlist: CloudflareClientOptions | undefined = getClient()?.getOptions()?.durableObjectSqlSpanAllowlist; |
There was a problem hiding this comment.
Yea probably, I merged before seeing your comments. I see this as a false positive so I just ignored the rule there.
| let currentFrames = getFramesFromEvent(currentEvent); | ||
| let previousFrames = getFramesFromEvent(previousEvent); | ||
| const currentFrames = getFramesFromEvent(currentEvent); | ||
| const previousFrames = getFramesFromEvent(previousEvent); |
3 participants
Upgrades oxlint (1.53 to 1.75) and oxlint-tsgolint (0.16 to 7, now on TS 7), and removes the
OXLINT_TSGOLINT_DANGEROUSLY_SUPPRESS_PROGRAM_DIAGNOSTICSworkaround from every lint script so real type-aware diagnostics run again.quotesis oxfmt's job now,import/no-unresolved,import/no-extraneous-dependencies,jsdoc/require-jsdoc,react/prop-types).no-return-awaitbecomestypescript/return-await.Also ignores some new rules like
unicornthat introduced a bunch of warnings, I see some useful stuff in there so I may re-enable them one by one later.One of the changes surfaced a very flakey test that relies on a microtick timing, i padded it to make it more reliable.