chore: modernize lint and tsconfig setup#34
Conversation
| ignores: [ | ||
| "node_modules/**", | ||
| "dist/**", | ||
| "coverage/**", | ||
| "test/**", | ||
| "karma.conf.ts", | ||
| "webpack.config.js", | ||
| "lib/**", | ||
| ".tmp/**", | ||
| ], |
There was a problem hiding this comment.
ignores list contains paths that don't exist in this repository
The list looks copy-pasted from a Power BI visual template. This repo is a utility library with only src/, lib/, and node_modules/ as code-bearing directories — dist/, coverage/, test/, karma.conf.ts, webpack.config.js, .tmp/ are not present.
Suggested trim:
ignores: [
"node_modules/**",
"lib/**",
],If the boilerplate entries are kept intentionally (e.g. anticipating a future test runner), a short comment would help future maintainers.
| "@typescript-eslint/eslint-plugin": "^8.59.4", | ||
| "@typescript-eslint/parser": "^8.59.4", | ||
| "eslint": "^10.4.0", | ||
| "eslint-plugin-powerbi-visuals": "^1.1.1" |
There was a problem hiding this comment.
typescript is not declared as a devDependency
npm run build calls tsc directly, but typescript is not listed in devDependencies. It currently resolves only because it's pulled in transitively by @typescript-eslint/* (today typescript@6.0.3). That makes the build:
- Non-reproducible — any transitive change can swap the compiler under us.
- Implicit — readers can't tell which TS version the library targets.
Since this PR explicitly modernizes to TS 6 (mentioned in the CHANGELOG), it's the right time to pin it. Suggested addition:
"devDependencies": {
...
"typescript": "^6.0.0"
}(Pre-existing on main, but worth fixing as part of the toolchain modernization.)
| "include": [ | ||
| "src/**/*.ts" | ||
| ] |
There was a problem hiding this comment.
Nit: inconsistent indentation around include
"include": [
"src/**/*.ts"
]The compilerOptions block uses 6-space indentation while this new include block uses 3 spaces before the key and 4 before the closing ]. Valid JSON, but visually broken. Suggested normalization to match the rest of the file:
"include": [
"src/**/*.ts"
]Also worth ending the file with a trailing newline (the previous version was missing one and the new one is too).
There was a problem hiding this comment.
Lint script uses the removed --ext flag
The "lint" script still passes --ext .js,.jsx,.ts,.tsx:
"lint": "npx eslint . --ext .js,.jsx,.ts,.tsx"ESLint 9 removed the --ext CLI flag together with .eslintrc support; ESLint 10 (pinned in this PR) has no use for it either. The script still appears to work only because eslint-plugin-powerbi-visuals@1.1.1's recommended preset already declares files: ["**/*.{js,jsx,ts,tsx}"], so the flag is silently ignored. It will start failing on a future minor bump or if the preset's files glob changes.
Could you simplify to:
"lint": "eslint ."(npx is also unnecessary in npm scripts — node_modules/.bin is already on PATH.)
|
|
||
| ### Changed | ||
| * Migrated ESLint config to flat-config ESM `eslint.config.mjs`. | ||
| * Removed legacy `.eslintrc.js` and `.eslintignore` in favor of flat-config equivalents. |
There was a problem hiding this comment.
This bullet doesn't match the actual tsconfig changes
Two inaccuracies vs. the tsconfig.json shipped in this PR:
ignoreDeprecationsis not set anywhere in the newtsconfig.json. Either drop it from this bullet or add it to the config (TS 6 reports several deprecations from the previous setup —ignoreDeprecations: "6.0"is what this line implies).strict: trueis effectively disabled. Right next to it the PR setsstrictNullChecks: false,strictPropertyInitialization: false,noImplicitAny: false, which turns off the three biggest strict-mode checks. Advertising "strict" here is misleading — consumers won't get strict-mode guarantees. Either remove the claim or actually enable the sub-flags.
| "module": "ES2020", | ||
| "moduleResolution": "bundler", |
There was a problem hiding this comment.
Mismatched module / moduleResolution pair
Per the TypeScript Modules Reference:
--moduleResolution bundlermust be paired with--module esnextor--module preserve.
The current "module": "ES2020" + "moduleResolution": "bundler" combination is technically accepted by tsc today but is not an officially supported pair — a future TS minor may emit TS5095.
Since the utils are moving to Vite (also a bundler), bundler resolution is the right choice. Suggested fix:
"module": "preserve",
"moduleResolution": "bundler",
"target": "es2022""preserve" (TS 5.4+) is preferred over "esnext" because it lets a file mix import and import x = require(...) if ever needed, and it matches what modern bundlers (Vite/Rollup/esbuild) actually do. Bumping target to es2022 aligns with what nodenext/modern Node already supports and avoids unnecessary downleveling.
No description provided.