feat: Add ESLint rule to standardise file extensions on imports #1541
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
rjmunro
force-pushed
the
rjmunro/improve-eslint
branch
from
95848d1 to
33949da
Compare
Julusian
left a comment
Julusian
left a comment
There was a problem hiding this comment.
It looks like for the backend code there is already a rule in place for this. Perhaps this should use the same plugin instead of adding another one?
I believe that rule is https://github.com/eslint-community/eslint-plugin-n/blob/master/docs/rules/file-extension-in-import.md
That plugin was omitted from the webui as its not using node at runtime, so some of the rules dont make sense. But maybe there are a handful of rules in that plugin which do make sense to enable?
packages/webui/eslint.config.mjs
Outdated
| '@typescript-eslint/unbound-method': 'off', | ||
| '@typescript-eslint/no-misused-promises': 'off', | ||
| '@typescript-eslint/no-unnecessary-type-assertion': 'off', | ||
| 'import-extensions/require-extensions': ['error', { expectedExtensions: ['js'] }], |
There was a problem hiding this comment.
These shouldn't be added here, as these are permanent additions, not Temporary rules to be removed over time
This was confusing ESLint because it effectively overrode packages/webui/src/client/ui/Collections.ts
By running eslint --fix fixup! chore: Ensure all imports have .js extension where applicable
Needed on Linux macOS and other unix-like systems.
rjmunro
force-pushed
the
rjmunro/improve-eslint
branch
from
ec01055 to
09eb0c6
Compare
WalkthroughThe PR adds eslint-plugin-n ESLint configuration to enforce explicit file extensions in imports, then systematically updates approximately 35 import statements across the webui client package to include .js/.jsx extensions for local module resolution. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/webui/eslint.config.mjs:
- Around line 29-31: The tryExtensions array in the configuration object (the n:
{ tryExtensions: [...] } block) is missing the '.jsx' entry; update that
tryExtensions list to include '.jsx' (e.g., add '.jsx' alongside other
extensions) so JSX imports are resolvable by the plugin and consistent with the
PR's enforcement of .jsx imports.
🧹 Nitpick comments (3)
packages/webui/src/client/lib/__tests__/lib.test.ts (1)
1-1: Remove stale comment.The comment
// Adjust the import path as neededappears to be a leftover reminder from when the path needed adjustment. Since the path has now been updated, this comment should be removed.🧹 Suggested cleanup
-import { useCurrentTime } from '../lib.js' // Adjust the import path as needed +import { useCurrentTime } from '../lib.js'packages/webui/src/client/ui/RundownView/RundownHeader/RundownHeader.tsx (1)
15-17: Consider consolidating imports from the same module.Two separate imports from
../../../lib/lib.jscould be combined into a single import statement for clarity.♻️ Suggested consolidation
-import { useRundownViewEventBusListener } from '../../../lib/lib.js' import { RundownLayoutRundownHeader } from '@sofie-automation/meteor-lib/dist/collections/RundownLayouts' -import { contextMenuHoldToDisplayTime } from '../../../lib/lib.js' +import { useRundownViewEventBusListener, contextMenuHoldToDisplayTime } from '../../../lib/lib.js'packages/webui/eslint.config.mjs (1)
15-15: Consider whether this rule belongs intmpRules.The comment indicates
tmpRulescontains "Temporary rules to be removed over time," butn/file-extension-in-importappears to be a permanent enforcement rule for ES module conventions rather than a temporary workaround.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.husky/pre-commitpackages/webui/eslint.config.mjspackages/webui/src/client/lib/Components/BreadCrumbTextInput.tsxpackages/webui/src/client/lib/Components/MultiLineIntInput.tsxpackages/webui/src/client/lib/Moment.tsxpackages/webui/src/client/lib/VirtualElement.tsxpackages/webui/src/client/lib/__tests__/lib.test.tspackages/webui/src/client/ui/Collections/index.tspackages/webui/src/client/ui/PreviewPopUp/Previews/LayerInfoPreview.tsxpackages/webui/src/client/ui/RundownView.tsxpackages/webui/src/client/ui/RundownView/CasparCGRestartButtons.tsxpackages/webui/src/client/ui/RundownView/RundownDetachedShelf.tsxpackages/webui/src/client/ui/RundownView/RundownHeader/RundownHeader.tsxpackages/webui/src/client/ui/RundownView/RundownHeader/RundownReloadResponse.tspackages/webui/src/client/ui/RundownView/RundownHeader/TimingDisplay.tsxpackages/webui/src/client/ui/RundownView/RundownHeader/useRundownPlaylistOperations.tsxpackages/webui/src/client/ui/RundownView/RundownSorensenContext.tsxpackages/webui/src/client/ui/RundownView/RundownViewContextProviders.tsxpackages/webui/src/client/ui/RundownView/RundownViewSubscriptions.tspackages/webui/src/client/ui/RundownView/WarningDisplay.tsxpackages/webui/src/client/ui/RundownView/useQueueMiniShelfAdlib.tspackages/webui/src/client/ui/Settings/Studio/PackageManager/AccessorTableRow.tsxpackages/webui/src/client/ui/Shelf/Shelf.tsxpackages/webui/src/client/ui/TestTools/DeviceTriggers.tsxpackages/webui/src/client/ui/TestTools/IngestRundownStatus.tsxpackages/webui/src/client/ui/TestTools/Mappings.tsxpackages/webui/src/client/ui/TestTools/Timeline.tsxpackages/webui/src/client/ui/TestTools/TimelineDatastore.tsxpackages/webui/src/client/ui/TestTools/collections.tspackages/webui/src/client/ui/TestTools/index.tsx
⏰ 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: Test Package (main) (22.x, corelib, true)
- GitHub Check: Test Package (main) (22.x, job-worker, true)
- GitHub Check: Publish Docs
🔇 Additional comments (33)
packages/webui/src/client/ui/PreviewPopUp/Previews/LayerInfoPreview.tsx (1)
2-2: LGTM!The import path correctly updated to include the explicit
.jsextension, consistent with the new ESLint rule being enforced across the webui package.packages/webui/src/client/lib/Components/MultiLineIntInput.tsx (1)
1-1: LGTM!The import path correctly includes the
.jsextension, aligning with the new ESLint rule for explicit file extensions. This is the proper approach for ES module resolution with compiled TypeScript.packages/webui/src/client/lib/VirtualElement.tsx (1)
3-3: LGTM!The explicit
.jsextension aligns with ES module resolution requirements. TypeScript correctly resolves.jsimports to their corresponding.ts/.tsxsource files during compilation.packages/webui/src/client/ui/RundownView.tsx (1)
30-30: LGTM!The import path updates to include explicit
.jsextensions are consistent with the rest of the file and align with the PR's goal of standardizing ES module resolution.Also applies to: 41-41
packages/webui/src/client/ui/TestTools/index.tsx (1)
12-12: LGTM!The
.jsxextension is appropriate for this import, consistent with the file type convention used in this codebase.packages/webui/src/client/lib/Components/BreadCrumbTextInput.tsx (1)
25-25: LGTM!The import path update to include the explicit
.jsxextension is consistent with the PR's module resolution standardization effort.packages/webui/src/client/ui/RundownView/WarningDisplay.tsx (1)
5-6: LGTM!The import path updates to include explicit
.jsextensions are consistent with the ES module resolution standardization across the codebase.packages/webui/src/client/ui/TestTools/IngestRundownStatus.tsx (1)
1-11: LGTM!The import path updates correctly add explicit file extensions (
.jsxfor JSX files,.jsfor TypeScript-compiled modules), aligning with ES module resolution conventions enforced by the new ESLint rule.packages/webui/src/client/ui/Settings/Studio/PackageManager/AccessorTableRow.tsx (1)
20-20: LGTM!The import path update for
IntInputControlnow includes the.jsextension, consistent with the other component imports in this file.packages/webui/src/client/ui/TestTools/collections.ts (1)
2-2: LGTM!The import path update correctly adds the
.jsextension for the compiled TypeScript module, consistent with ES module resolution.packages/webui/src/client/ui/TestTools/DeviceTriggers.tsx (1)
2-2: LGTM!The import path updates correctly add explicit extensions (
.jsxfor the React Meteor data hooks,.jsfor the local collections module), maintaining consistency with the ES module resolution pattern.Also applies to: 11-11
packages/webui/src/client/ui/TestTools/Timeline.tsx (1)
29-29: LGTM!The import path update for
StudioTimelinenow includes the.jsextension, consistent with the other imports in this file.packages/webui/src/client/ui/TestTools/Mappings.tsx (1)
14-14: LGTM!The import extension update to
./collections.jsaligns with the PR's ESLint rule enforcement for explicit file extensions.packages/webui/src/client/ui/RundownView/RundownHeader/TimingDisplay.tsx (1)
6-14: LGTM!All import paths correctly updated with explicit
.jsextensions, consistent with the ES module resolution standardization.packages/webui/src/client/lib/Moment.tsx (1)
3-3: LGTM!Import path correctly updated with explicit
.jsextension.packages/webui/src/client/ui/RundownView/RundownHeader/useRundownPlaylistOperations.tsx (7)
4-6: LGTM!Import paths correctly updated with explicit
.jsextensions for clientUserAction, meteorApi, and ModalDialog modules.
9-9: LGTM!Import path for UserPermissions correctly updated.
12-12: LGTM!Import path for logging correctly updated.
14-14: LGTM!Import path for notifications correctly updated.
18-20: LGTM!Import paths for RundownReloadResponse, viewPort, and lib correctly updated.
23-23: LGTM!Import path for systemTime correctly updated.
25-25: LGTM!Import path for WarningDisplay correctly updated.
packages/webui/src/client/ui/RundownView/RundownSorensenContext.tsx (1)
6-13: LGTM!Import paths correctly updated with explicit
.jsextensions. All imports reference real files in the codebase.packages/webui/src/client/ui/TestTools/TimelineDatastore.tsx (1)
1-2: LGTM!The import path updates correctly apply explicit file extensions (
.jsxfor React component files and.jsfor collection modules), aligning with the ESLint standardization effort.Also applies to: 10-10
packages/webui/src/client/ui/RundownView/RundownDetachedShelf.tsx (1)
7-13: LGTM!The import paths are correctly updated with explicit
.jsextensions for all local module imports, consistent with the project-wide ESM standardization.packages/webui/src/client/ui/RundownView/useQueueMiniShelfAdlib.ts (1)
4-6: LGTM!The import paths are correctly updated with explicit
.jsextensions for all local module imports, aligning with the ESLint standardization effort.Also applies to: 8-8, 20-23
packages/webui/src/client/ui/RundownView/RundownViewContextProviders.tsx (1)
2-6: LGTM!The import paths are correctly updated with explicit
.jsextensions, maintaining consistency with the project-wide ESM standardization.Also applies to: 8-8
packages/webui/src/client/ui/RundownView/CasparCGRestartButtons.tsx (1)
10-15: LGTM!The import paths are correctly updated with explicit
.jsextensions. Usingindex.jsexplicitly for the collections import (line 10) is a good practice for ESM resolution clarity.packages/webui/src/client/ui/RundownView/RundownHeader/RundownHeader.tsx (1)
5-5: LGTM!The
.jsextensions on relative imports align with ES module resolution requirements. The import paths are correctly structured relative to the file location.Also applies to: 11-14, 24-24, 26-28, 32-37
packages/webui/eslint.config.mjs (1)
25-27: LGTM!The plugin is correctly registered in the flat config format.
packages/webui/src/client/ui/RundownView/RundownHeader/RundownReloadResponse.ts (1)
1-1: LGTM!The
.jsextensions are correctly applied to all relative imports, including theindex.jsbarrel file reference.Also applies to: 7-7, 9-10, 15-17
packages/webui/src/client/ui/RundownView/RundownViewSubscriptions.ts (1)
6-6: LGTM!The
.jsextensions are correctly applied to all relative imports, maintaining consistency with the ESLint rule configuration.Also applies to: 11-11, 13-14, 16-16
packages/webui/src/client/ui/Shelf/Shelf.tsx (1)
44-48: LGTM! Import extensions standardized correctly.The addition of explicit
.jsextensions to these three relative imports is correct and consistent with the ES module pattern used throughout the rest of the file. All relative imports now include extensions while external package imports correctly omit them.
| n: { | ||
| tryExtensions: ['.js', '.cjs', '.mjs', '.json', '.node', '.ts', '.cts', '.mts', '.d.ts', '.tsx'], | ||
| }, |
There was a problem hiding this comment.
Missing .jsx extension in tryExtensions.
The PR objective mentions enforcing .jsx extensions on imports, but .jsx is not included in the tryExtensions list. This may cause the plugin to fail to resolve JSX imports correctly.
🔧 Suggested fix
n: {
- tryExtensions: ['.js', '.cjs', '.mjs', '.json', '.node', '.ts', '.cts', '.mts', '.d.ts', '.tsx'],
+ tryExtensions: ['.js', '.jsx', '.cjs', '.mjs', '.json', '.node', '.ts', '.cts', '.mts', '.d.ts', '.tsx'],
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| n: { | |
| tryExtensions: ['.js', '.cjs', '.mjs', '.json', '.node', '.ts', '.cts', '.mts', '.d.ts', '.tsx'], | |
| }, | |
| n: { | |
| tryExtensions: ['.js', '.jsx', '.cjs', '.mjs', '.json', '.node', '.ts', '.cts', '.mts', '.d.ts', '.tsx'], | |
| }, |
🤖 Prompt for AI Agents
In @packages/webui/eslint.config.mjs around lines 29 - 31, The tryExtensions
array in the configuration object (the n: { tryExtensions: [...] } block) is
missing the '.jsx' entry; update that tryExtensions list to include '.jsx'
(e.g., add '.jsx' alongside other extensions) so JSX imports are resolvable by
the plugin and consistent with the PR's enforcement of .jsx imports.
|
@Julusian I tried using the other plugin. It incorrectly added a few more .js extensions where the import was a folder which caused errors. I also noticed that metor has: in it's eslint config, i.e. disabling all .js extensions in imports. I wonder why we have it set at all in sofie-standard-code-preset. I'd rather remove them all globally. I think it's much neater, especially as the mostly aren't js files, they are ts or tsx files that are complied to js files. Are they actually needed anywhere? |
For any packages which are actually generated as ESM, the extensions are necessary. So I thought it easier to enforce everywhere rather than having that variation based on how the project is generated.
Yeah, in ESM you can't import a folder, those imports will need to be adjusted to point to the index file within the folder |
Can you not configure typescript or a bundler or something to do that at compile time? |
|
typescript will not (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-7.html#path-rewriting-for-relative-paths talks about some ts->js conversion, but states it doesnt change extensionless) Most bundler probably can, but then we would need to deal with the complexity of that extra build step. |
3 participants
About the Contributor
Type of Contribution
This is a code improvement
Current Behavior
Between release52 and release53, particularly in d02f416, the standard for imports seemed to be changed to include
.json the end of the filename. This caused merge conflicts when I was trying to rebase a branch targeted at release52 onto release53.New Behavior
Use an eslint plugin to enforce extensions on file imports. This enabled me to automatically resolve a bunch of merge conflicts using my git resolve-formatting-conflicts tool.
Currently I have applied this is just to the webui package because that's where I was working. It could probably be done more widely.
Testing
Affected areas
This PR affects code formatting and it's enforcement.
Time Frame
Other Information
It looks like there is a script to do the modification here: 88c500f. The advantage of this is that it is part of normal ESLint formatting, so IDEs and will highlight and auto-correct, CI will catch it, and it will work with tools like git resolve-formatting-conflicts without any further customisation.
Status
Summary
This pull request introduces an ESLint rule to standardize and enforce file extensions (
.js,.jsx) on import statements within the webui package, addressing recurring merge conflicts caused by inconsistent import formats between release versions.Key Changes
ESLint Configuration
eslint-plugin-nto the ESLint configuration inpackages/webui/eslint.config.mjsn/file-extension-in-importrule set to 'error' to enforce extensions on importstryExtensionsincluding.js,.cjs,.mjs,.json,.node,.ts,.cts,.mts,.d.ts, and.tsxImport Path Updates
Systematically updated import statements across approximately 40 files in
packages/webui/src/client/to include explicit file extensions:.jsextensions (e.g.,'./lib'→'./lib.js','../../collections'→'../../collections/index.js').jsxor.jsextensions as appropriateImpact