Fix ESLint errors in component TypeScript - #520
Conversation
Marks SelectStrategy and SelectVariant as type-only imports so import/named resolves them, and removes stale eslint-disable comments referencing custom-elements rules that eslint-plugin-github v6 no longer ships. Exports CollapsibleHeaderElement and CollapsibleSectionElement, which were flagged by no-unused-vars because their only reference was the Window interface in declare global. Exporting follows the idiom of AvatarFallbackElement and FilterableTreeViewElement, and avoids the redundant window.customElements.get guard other components carry: Catalyst's @controller already registers the element and assigns window[className], so that guard is never true. All seven errors pre-date this branch; this makes npm run lint green.
|
There was a problem hiding this comment.
Pull request overview
This PR cleans up TypeScript in Primer/OpenProject components to remove accumulated ESLint errors (notably after eslint-plugin-github v6), restoring npm run lint as a reliable signal without changing intended runtime behavior.
Changes:
- Convert
SelectStrategy/SelectVariantimports in TreeView totypeimports to satisfyimport/named. - Remove now-invalid inline
eslint-disabledirectives for rules that are no longer shipped by the plugin. - Export Catalyst controller classes that are otherwise only referenced in type positions (to satisfy
@typescript-eslint/no-unused-vars).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| app/components/primer/open_project/zen_mode_button.ts | Removes an invalid eslint-disable comment around the custom element constructor. |
| app/components/primer/open_project/collapsible_section.ts | Exports the controller class to avoid no-unused-vars when used only in declare global. |
| app/components/primer/open_project/border_box/collapsible_header.ts | Exports the controller class to avoid no-unused-vars when used only in declare global. |
| app/components/primer/alpha/tree_view/tree_view.ts | Marks SelectStrategy/SelectVariant as type-only imports and removes invalid eslint-disable directives. |
Suppressed comments (1)
app/components/primer/open_project/zen_mode_button.ts:14
addEventListener/removeEventListenerare usingthis.fullscreenChangeEventHandler.bind(this)inline. Eachbindcall creates a new function reference, so the listener added in the constructor is never removed indisconnectedCallback, which can leak listeners and cause duplicate event handling when elements are re-created.
document.addEventListener('fullscreenchange', this.fullscreenChangeEventHandler.bind(this))
}
disconnectedCallback() {
document.removeEventListener('fullscreenchange', this.fullscreenChangeEventHandler.bind(this))
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🤖 Opened by an agent on Alex's behalf.
What are you trying to accomplish?
npm run lintcurrently reports 7 ESLint errors onmain. None of them are new; they accumulated aseslint-plugin-githubmoved to v6 and dropped rules the codebase still disables inline. This clears all of them so lint is a meaningful signal again, and so branches stacked on top start from a green baseline.The errors:
import/named—SelectStrategyandSelectVariantare types, not values, soimport/namedcannot resolve them from./tree_view_sub_tree_node_element. Marked astypeimports.custom-elements/no-dom-traversal-in-connectedcallback(×2) andcustom-elements/no-constructor(×1) —eslint-disablecomments for ruleseslint-plugin-githubv6 no longer ships, so ESLint errors on the unknown rule name. Removed.@typescript-eslint/no-unused-vars(×2) —CollapsibleHeaderElementandCollapsibleSectionElementare referenced only from theWindowinterface insidedeclare global, i.e. only in type position. Exported them.What approach did you choose and why?
For the two
no-unused-varserrors,export classfollows the idiom already used byAvatarFallbackElementandFilterableTreeViewElement. Sinceprimer.tsimports these modules for their side effects only, and neither is re-exported from the package entry point, the published API surface is unchanged.The alternative — the
if (!window.customElements.get('…')) { window.X = X; window.customElements.define('…', X) }block that several other components carry — was deliberately not used. With@controller, Catalyst'sregister()already callscustomElements.defineand assignswindow[classObject.name](swallowing theNotSupportedErrorfor a duplicate definition), so by the time the module body runs the element is registered and the guard is always false. That block is dead code wherever it appears alongside@controller.Removing the existing dead blocks is left out of scope on purpose: most of the files carrying them (
alpha/*) come from upstreamprimer/view_components, and diverging there would create conflicts on every upstream sync.Anything you want to highlight for special attention from reviewers?
Validation performed:
npm run lint— 0 errors (stylelint + eslint)npx tsc --noEmit— cleanscript/build-assets js— bundle buildsAccessibility
Merge checklist
#515 is stacked on this PR (base
fix/eslint-component-typescript). Merge this one first; #515 then retargets tomain.