chore(eslint): Enable TypeScript resolver for eslint-plugin-import #36934
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds ESLint disable comments to several Meteor auth override and server import lines; updates eslint-config to group external/internal imports together and enables a TypeScript import resolver by adding resolver settings and a new dependency. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/meteor/overrides/login/meteorDeveloperAccount.ts (1)
22-23: Missing Random import (runtime/TS error)
Random.secret()is used butRandomisn’t imported in this file. Align with other providers.+import { Random } from '@rocket.chat/random';
♻️ Duplicate comments (4)
apps/meteor/client/meteor/overrides/login/twitter.ts (1)
4-7: Avoid inline disables for import/no-duplicatesSame suggestion as in google.ts: prefer a config-level ignore for
meteor/*modules so these lines aren’t needed.apps/meteor/client/meteor/overrides/login/meteorDeveloperAccount.ts (1)
2-5: Avoid inline disables for import/no-duplicatesSame as other overrides; handle Meteor modules in ESLint config and remove these comments.
apps/meteor/client/meteor/overrides/login/github.ts (1)
3-7: Avoid inline disables for import/no-duplicatesCentralize the Meteor handling in ESLint config; then remove these disables.
apps/meteor/client/meteor/overrides/login/facebook.ts (1)
3-7: Avoid inline disables for import/no-duplicatesHandle Meteor packages in ESLint config and remove these comments (same as other override files).
🧹 Nitpick comments (8)
apps/meteor/client/meteor/overrides/login/google.ts (2)
3-7: Prefer config-level fix over scattered inline disablesInstead of per-line
import/no-duplicatesdisables, address Meteor packages globally in the ESLint config (see comment on packages/eslint-config/imports/index.js). This lets you drop these comments repo-wide.
83-103: Remove redundant Object.assign of the same params
loginUrlParametersis built with the properties already, then immediately reassigned with the same keys. Drop the second assignment.- Object.assign(loginUrlParameters, { - response_type: 'code', - client_id: config.clientId, - scope, - redirect_uri: OAuth._redirectUri('google', config), - state: OAuth._stateParam(loginStyle, credentialToken, options.redirectUrl), - });apps/meteor/app/api/server/ApiClass.ts (2)
16-19: Combine type and value imports to avoid duplicate-import ruleYou can import both type and value from the same module and drop the disable.
-import type { RateLimiterOptionsToCheck } from 'meteor/rate-limit'; -// eslint-disable-next-line import/no-duplicates -import { RateLimiter } from 'meteor/rate-limit'; +import { type RateLimiterOptionsToCheck, RateLimiter } from 'meteor/rate-limit';
13-14: Unnecessary disable?
ddpandddp-commonare distinct modules; if lint still flags duplicates, prefer a config tweak over per-line disables (see config comment). Otherwise, remove this comment.packages/eslint-config/imports/index.js (1)
11-20: Centralize Meteor handling to eliminate inline disablesIgnore Meteor package paths at the resolver layer so
import/no-duplicatesand friends don’t misfire onmeteor/*imports across the codebase.settings: { 'import/resolver': { node: { extensions: ['.mjs', '.js', '.json'], }, }, 'import/extensions': ['.js', '.mjs', '.jsx'], 'import/core-modules': [], - 'import/ignore': ['node_modules', '\\.(coffee|scss|css|less|hbs|svg|json)$'], + 'import/ignore': ['node_modules', '\\.(coffee|scss|css|less|hbs|svg|json)$', '^meteor/.+$'], },packages/eslint-config/standard/index.js (3)
125-125: Resolve @types packages automaticallyEnable
alwaysTryTypesso the resolver also finds@types/*packages, reducing false positives for type-only imports.Apply this diff:
- typescript: {}, + typescript: { alwaysTryTypes: true },
121-126: Consider moving resolver config to top-level settings for consistencyRight now the TS resolver is only active for
**/*.ts(x)files. If JS files import modules via TS path aliases (through Babel plugins or runtime aliasing),import/no-unresolvedmay still fire. Consider defining the same resolver at the top-levelsettingsto cover mixed codebases.Example (outside this hunk, near Lines 7-13):
settings: { 'import/resolver': { node: { extensions: ['.js', '.mjs', '.cjs', '.ts', '.tsx'] }, typescript: { alwaysTryTypes: true }, }, },
123-124: Optional: include ESM/CJS extensionsIf the repo contains
.mjs/.cjsfiles, include them to avoidimport/no-unresolvednoise.Apply this diff:
- node: { - extensions: ['.js', '.ts', '.tsx'], - }, + node: { + extensions: ['.js', '.mjs', '.cjs', '.ts', '.tsx'], + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
apps/meteor/app/api/server/ApiClass.ts(1 hunks)apps/meteor/client/meteor/overrides/login/facebook.ts(1 hunks)apps/meteor/client/meteor/overrides/login/github.ts(1 hunks)apps/meteor/client/meteor/overrides/login/google.ts(1 hunks)apps/meteor/client/meteor/overrides/login/meteorDeveloperAccount.ts(1 hunks)apps/meteor/client/meteor/overrides/login/twitter.ts(1 hunks)packages/eslint-config/imports/index.js(1 hunks)packages/eslint-config/package.json(1 hunks)packages/eslint-config/standard/index.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
packages/eslint-config/package.json (1)
14-14: Verify TypeScript import resolver is active for consumerspackages/eslint-config/package.json declares eslint-import-resolver-typescript (~4.4.4) and packages/eslint-config/standard/index.js enables the "typescript" resolver in the TS override, but several consumer .eslintrc.json files override settings with "import/resolver": node (examples: packages/ui-client/.eslintrc.json, packages/ui-composer/.eslintrc.json, packages/ui-avatar/.eslintrc.json, packages/fuselage-ui-kit/.eslintrc.json, packages/livechat/.eslintrc.json).
Run locally to confirm for a real TS file:
eslint --print-config path/to/consumer/file.ts | jq '.settings["import/resolver"]'If the output does not include "typescript", remove/merge the local "import/resolver" entries or explicitly add "typescript": {} to the consuming config.
packages/eslint-config/imports/index.js (1)
151-151: Merging external and internal groups LGTMCombining them should reduce noisy diffs across the monorepo.
packages/eslint-config/standard/index.js (1)
121-126: TS resolver addition — LGTMAdding
typescriptunderimport/resolverinside the TS override will fix manyimport/no-unresolvedcases for path aliases and TS files.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36934 +/- ##
===========================================
- Coverage 66.54% 66.53% -0.02%
===========================================
Files 3344 3344
Lines 114629 114629
Branches 21094 21093 -1
===========================================
- Hits 76281 76269 -12
- Misses 35658 35667 +9
- Partials 2690 2693 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
tassoevan
force-pushed
the
chore/eslint-typescript-resolution
branch
from
625da7f to
9b1c295
Compare
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
ARCH-1810
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Style
Chores
User Impact