INTER-2281: upgrade dx-team-toolkit dev dependencies to latest#249
INTER-2281: upgrade dx-team-toolkit dev dependencies to latest#249JuroUhlar wants to merge 16 commits into
Conversation
🦋 Changeset detectedLatest commit: 86c5349 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Coverage report
Test suite run success79 tests passing in 24 suites. Report generated by 🧪jest coverage report action from 86c5349 Show full coverage report
|
There was a problem hiding this comment.
Pull request overview
Upgrades the repo’s DX team toolkit dev dependencies and migrates ESLint from legacy .eslintrc to ESLint 10 flat config, with small source edits to satisfy the stricter lint rules.
Changes:
- Upgraded
@fingerprintjs/*-dx-teamtooling packages and added expliciteslint@10+prettier@3. - Migrated ESLint configuration to
eslint.config.jsand adjusted thelintscript accordingly. - Applied minor code tweaks / lint suppressions (nullish checks, assertion-related rule overrides, formatting).
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/urlUtils.ts | Replaces == null with explicit `=== null |
| src/serverApiClient.ts | Adds documented eslint suppressions for type assertions used in option normalization and JSON parsing. |
| src/sealedResults.ts | Adds eslint suppression + commentary around caught-error assertion in unsealing flow. |
| src/errors/toError.ts | Adds eslint suppression + commentary around returning non-Error objects as Error. |
| src/errors/handleErrorResponse.ts | Formatting/indentation adjustment in the error-response type guard. |
| rollup.config.js | Suppresses no-require-imports for require('./package.json'). |
| package.json | Updates lint script; bumps DX-team tooling deps; adds explicit eslint and prettier. |
| eslint.config.js | Introduces ESLint flat config with ignores and test-file rule relaxations. |
| .eslintrc.json | Removes legacy ESLint config file. |
| pnpm-lock.yaml | Lockfile updates reflecting new toolchain versions and dependency graph. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Upgrade the shared FingerprintJS DX team toolkit dev dependencies: - @fingerprintjs/eslint-config-dx-team: 0.1.0 -> 2.0.0 - @fingerprintjs/conventional-changelog-dx-team: 0.1.0 -> 0.2.0 - @fingerprintjs/prettier-config-dx-team: 0.2.0 -> 0.3.0 The new eslint config is a flat config built on ESLint 10 and typescript-eslint 8, so migrate accordingly: - Replace .eslintrc.json with eslint.config.js (flat config) - Add explicit eslint and prettier dev dependencies - Drop the removed --ext / --ignore-path flags from the lint script - Relax consistent-type-assertions / no-explicit-any for test files (mocking legitimately relies on assertions such as `fetch as unknown as jest.Mock`) Fix issues surfaced by the stricter ruleset: - Use strict equality for nullish checks in urlUtils - Add explained eslint-disable directives for the few unavoidable type assertions in source (assertionStyle: never) and the require() import in rollup.config.js - Apply prettier 3.9 formatting Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RRhyyBoYDJH5Aoq8MqXCNF
Address Copilot review feedback on the type-assertion suppressions: - toError: return the value directly when it is already an Error, and otherwise construct a real Error (preserving the message for error-like objects that are not Error instances, e.g. cross-realm values from native bindings). Removes the misleading "safe" assertion. - sealedResults: normalize the caught value via toError instead of asserting `e as Error`, so UnsealError always receives a real Error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RRhyyBoYDJH5Aoq8MqXCNF
719a58c to
bfc8d9f
Compare
This comment has been minimized.
This comment has been minimized.
Switch to @fingerprintjs/eslint-config-dx-team/type-checked and remove eslint from devDependencies since the preset bundles it.
Adjust type guards and sealedResults switch, extend test/example eslint overrides, and restore region cast in serverApiClient.
bfc8d9f to
23e6a76
Compare
Add tooling tsconfig for vitest/generate, trim test rule overrides, fix lint issues in tests and generate.mts, and switch eslint config to ESM.
Point ESLint at tests/tsconfig.json so Vitest types resolve in the IDE.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
6a3b1bb to
2bfde62
Compare
| this.region = (options.region as Region) ?? Region.Global | ||
| // Options.region accepts string literals for the same runtime values as the enum. | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- string literal union is not assignable to Region | ||
| this.region = (options.region ?? Region.Global) as Region |
There was a problem hiding this comment.
nit: We may still use @typescript-eslint/consistent-type-assertions here by using const instead of enum.
// src/types.ts
export const Region = {
EU: 'EU',
AP: 'AP',
Global: 'Global',
} as const
export type Region = (typeof Region)[keyof typeof Region]
Lets consistent-type-assertions stay enabled without a disable in serverApiClient, since options.region is now assignable to Region directly. Runtime output is identical to the string enum.
The const object union narrows `let region = Region.Global` to the "Global" literal, so reassigning Region.EU/AP failed typecheck:examples. JSDoc @type {Region} widens it back.
| return new Error(String(error.message)) | ||
| } | ||
|
|
||
| return new Error(String(error)) |
There was a problem hiding this comment.
If the error object doesn't have a toString function, String(error) will result in [object Object]. Since message is checked as being a property on error, perhaps this should use message somehow?
There was a problem hiding this comment.
I am not sure I follow, if message exists the conditional code block above returns the error using error.message. This line (13) only executes if message does not exist, no?
Or are you proposing something like this?
| return new Error(String(error)) | |
| return new Error(JSON.stringify(error)) |
There was a problem hiding this comment.
I am not sure I follow, if message exists the conditional code block above returns the error using error.message. This line (13) only executes if message does not exist, no?
Sorry! I misread the diff because the "Branch is not covered" annotations made it difficult to read. I've found the "Show annotations" setting and disabled it now 🤦 .
Or are you proposing something like this?
I think we should probably only use the result of String(error) if it'll provide a meaningful message. So, maybe the check can be something like?
const message = String(error)
if (message !== '[object Object]') {
return new Error(message)
}
return new Error(JSON.stringify(error))
There was a problem hiding this comment.
I like that, thank you! Also added basic tests to cover this case: 86c5349
🚀 Following releases will be created using changesets from this PR:@fingerprint/node-sdk@7.4.1Patch Changes
|
Errorcauses; includes a patch changeset.Note
eslintis provided by the dx-team ESLint config;prettierstays direct because it is a peer dependency.Verified with
pnpm lint,pnpm test,pnpm build, and typecheck scripts.