Skip to content

Fix ESLint errors in component TypeScript - #520

Open
myabc wants to merge 1 commit into
mainfrom
fix/eslint-component-typescript
Open

Fix ESLint errors in component TypeScript#520
myabc wants to merge 1 commit into
mainfrom
fix/eslint-component-typescript

Conversation

@myabc

@myabc myabc commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

🤖 Opened by an agent on Alex's behalf.

What are you trying to accomplish?

npm run lint currently reports 7 ESLint errors on main. None of them are new; they accumulated as eslint-plugin-github moved 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/namedSelectStrategy and SelectVariant are types, not values, so import/named cannot resolve them from ./tree_view_sub_tree_node_element. Marked as type imports.
  • custom-elements/no-dom-traversal-in-connectedcallback (×2) and custom-elements/no-constructor (×1) — eslint-disable comments for rules eslint-plugin-github v6 no longer ships, so ESLint errors on the unknown rule name. Removed.
  • @typescript-eslint/no-unused-vars (×2) — CollapsibleHeaderElement and CollapsibleSectionElement are referenced only from the Window interface inside declare global, i.e. only in type position. Exported them.

What approach did you choose and why?

For the two no-unused-vars errors, export class follows the idiom already used by AvatarFallbackElement and FilterableTreeViewElement. Since primer.ts imports 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's register() already calls customElements.define and assigns window[classObject.name] (swallowing the NotSupportedError for 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 upstream primer/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 — clean
  • script/build-assets js — bundle builds

Accessibility

  • No new axe scan violation — no rendered markup, focus behavior, or accessibility semantics change; element registration behavior is identical.

Merge checklist

  • Added/updated tests — not applicable; no behavior change
  • Added/updated documentation — not applicable
  • Added/updated previews (Lookbook) — not applicable

#515 is stacked on this PR (base fix/eslint-component-typescript). Merge this one first; #515 then retargets to main.

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.
Copilot AI lite review requested due to automatic review settings August 14, 2026 17:00
@changeset-bot

changeset-bot Bot commented Aug 14, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 6ce1691

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 / SelectVariant imports in TreeView to type imports to satisfy import/named.
  • Remove now-invalid inline eslint-disable directives 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/removeEventListener are using this.fullscreenChangeEventHandler.bind(this) inline. Each bind call creates a new function reference, so the listener added in the constructor is never removed in disconnectedCallback, 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.

@myabc myabc added the skip changeset For PRs that contain no relevant release data (e.g. documentation only) label Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip changeset For PRs that contain no relevant release data (e.g. documentation only)

Development

Successfully merging this pull request may close these issues.

2 participants