fix(desktop): externalize @wavegrid/doctor so main.js stops requiring ws itself - #90
Merged
Conversation
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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.
Summary
pnpm startinpackages/desktopdied at launch withCannot find module 'ws'(regression from #88).@wavegrid/doctorwas missing fromvite.main.config.ts's rollupexternallist, so Rollup inlined it intomain.js. Its ownimport { WebSocket } from 'ws'then became a barerequire("ws")inside the desktop bundle, resolved frompackages/desktop— which never declaresws:It only worked on my machine because pnpm had hoisted a
wscopy into the workspace rootnode_modules, which shadowed the bug; a clean/differently-hoisted install (Dan's) has nothing to resolve.Fix is one line — add
'@wavegrid/doctor'toexternal, matching the other brain packages — plus__tests__/main-externals.test.ts, which parses theexternalarray out ofvite.main.config.tsand asserts every@wavegrid/*thatsrc/main*imports appears in it. This class of break is invisible totsc --noEmit, lint and the unit suite (all green while the app couldn't boot), so the guard is the point of the PR as much as the fix. Verified it fails when the line is removed.Link to Devin session: https://app.devin.ai/sessions/972698f89f494b86828010666a002b8f
Requested by: @pyramation