Fix webviews type errors and enforce svelte-check in CI - #885
Open
chrisdp wants to merge 3 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The webviews have a type checker (
npm run checkin webviews/, running svelte-check), but nothing ran it. The production build isvite build, which transpiles without type checking, the root build/preversion scripts never invoked svelte-check, and no CI workflow ran it either. It was failing on master with 17 errors, unnoticed because every broken import was type-only, so vite erased them and the runtime bundle was unaffected.Type errors
Two failure categories were involved:
webviews/src/views/RceManagementView/RceManagementView.svelteimportedFirmwareVersionOutandSnapshotOutfrom roku-deploy; those names don't exist (the real exports areFirmwareVersionandSnapshot). Renamed all usages to match.RceStateDevicefromsrc/viewProviders/RceManagementViewProvider.ts, which pulled the provider's entire extension-side import graph (BaseWebviewViewProvider, managers, util, etc.) into svelte-check's program. That code gets compiled underwebviews/tsconfig.json(@tsconfig/svelte,moduleResolution: "bundler",verbatimModuleSyntax), settings it was never written for, producing 15 interop errors in src/ files (dayjs/md5/JSZip "not callable",import x = require(...)rejections, Buffer mismatches).Rather than patching each src/ error individually, the fix cuts the import graph: the
RceStateDeviceinterface moved verbatim into a new leaf module,src/viewProviders/RceManagementViewContract.ts, whose only import isimport type { DeviceStatus, DeviceType } from 'roku-deploy'. The provider re-exports the type so extension-side importers are unchanged, and the webview imports the contract file directly instead of the provider.Warning cleanup
svelte-check also reported 211 warnings, so this PR takes it to zero so new problems stand out:
<vscode-divider />style) to explicit open/close pairs; the HTML parser ignores the slash on non-void elements, so the self-closing form is ambiguous and Svelte 5 warns on it.altattribute.node_invalid_placement_ssrnesting warnings. Most of the a11y hits are false positives onvscode-*toolkit custom elements, which are natively interactive but opaque to svelte-check's static analysis; the webviews never server-render, so the SSR warnings don't apply.Suppression is centralized in
webviews/svelte.config.jsvia Svelte 5'scompilerOptions.warningFilter. Unlikeonwarn(a build hook only vite invokes),warningFilteris a compiler option, so vite build, svelte-check, and the VS Code svelte extension all honor it from the one config file. The repo previously had two other suppression mechanisms (anonwarnfilter and asvelte.plugin.svelte.compilerWarningsmap in.vscode/settings.json), both silently dead since the Svelte 5 upgrade renamed every warning code from dashes to underscores; both are removed.Enforcement
A new root script
check-webviews(cd webviews && npm run check) is added to the CI build workflow between lint and test, and to thepreversionchain so releases can't ship with a red webviews typecheck. Rootnpm cialready installs webviews deps via postinstall, so CI needed no extra setup.Aside from the markup expansions and the
altattribute, every edit is type-level, config, or a type-only import move; there's no runtime change.