Fix building the @moq/publish and @moq/watch packages#957
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request reorganizes the monorepo's build infrastructure and introduces a new AudioWorklet handling system. Changes include: consolidating shared build scripts from 🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/hang-ui/package.json (1)
8-11:⚠️ Potential issue | 🟠 MajorMissing
typescondition inexports— thepackage.tsbuild script does not auto-inject it.The PR adds
tscto emit.d.tsfiles intodist/, but theexportsmap in package.json lacks atypescondition. Whilepackage.tsrewrites path values (transforming./src/publish/index.tsxto./publish/index.js), it does not generate missingtypesconditions. TypeScript consumers usingmoduleResolution: "bundler"or"node16"won't resolve the generated declarations.Update exports to include the
typescondition:"exports": { "./publish": { "types": "./dist/publish/index.d.ts", "default": "./publish/index.js" }, "./watch": { "types": "./dist/watch/index.d.ts", "default": "./watch/index.js" } }The
package.tsscript will automatically rewrite the nesteddefaultpaths during the build.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@js/scripts/package.ts`:
- Around line 28-33: The object-export branch in js/scripts/package.ts
incorrectly rewrites every string sub-value with .js; update the loop that
iterates over sub-keys of val so that when the key name is exactly "types" you
call rewritePath(val[sub], "d.ts") (or otherwise ensure the types path is
rewritten to a .d.ts extension), and for all other string sub-values continue
using rewritePath(val[sub], "js"); keep the existing typeof checks (if (typeof
val[sub] === "string")) and only change the argument passed to rewritePath based
on the sub key name.
🧹 Nitpick comments (1)
js/scripts/package.ts (1)
56-69: Workspace dependency resolution assumes a flat sibling layout.Line 63 builds
../${packageDir}/package.jsonwhich works for the currentjs/*workspace structure. However, if a scoped package name doesn't match its directory (e.g.,@moq/hang-uiliving injs/hang-ui), thename.split("/")[1]extraction ofhang-uistill works because the workspace directories match the unscoped portion.One edge-case: if
readFileSyncfails (e.g., the workspace package doesn't exist at the expected path), the error message won't be very helpful. Consider wrapping with a try/catch that names the dependency.💡 Optional: improve error diagnostics
if (typeof version === "string" && version.startsWith("workspace:")) { const packageDir = name.includes("/") ? name.split("/")[1] : name; const workspacePkgPath = `../${packageDir}/package.json`; - const workspacePkg = JSON.parse(readFileSync(workspacePkgPath, "utf8")); + let workspacePkg: { version: string }; + try { + workspacePkg = JSON.parse(readFileSync(workspacePkgPath, "utf8")); + } catch (e) { + throw new Error(`Failed to resolve workspace dependency "${name}" at ${workspacePkgPath}: ${e}`); + } dependencies[name] = `^${workspacePkg.version}`;
|
Blocked on #948 |
Update watch and publish packages to use emitDeclarationOnly tsc after vite build, matching the ui-core pattern. Remove unnecessary tsconfig.build.json and src/ui/tsconfig.json files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kixelated
changed the title
kixelated
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@js/common/vite-plugin-worklet.ts`:
- Around line 30-50: The load(id) handler in vite-plugin-worklet.ts doesn’t
register the original worklet source with Vite’s watcher, so edits won’t trigger
rebuilds; inside the async load(id) function (where filePath is derived from
id.slice(0, -SUFFIX.length)) call this.addWatchFile(filePath) (guarded by
checking this.addWatchFile exists) before invoking build so Vite tracks the
original file in dev/watch mode and rebuilds the virtual module when the source
changes.
🧹 Nitpick comments (2)
js/common/vite-plugin-worklet.ts (1)
43-43: Add a guard for missing esbuild output.If esbuild produces no output files (e.g., empty entry or misconfiguration),
result.outputFiles[0].textwill throw an opaqueTypeError. A quick guard with a descriptive error improves debuggability.Proposed fix
- const compiled = result.outputFiles[0].text; + const output = result.outputFiles?.[0]; + if (!output) { + throw new Error(`worklet-inline: esbuild produced no output for ${filePath}`); + } + const compiled = output.text;js/common/package.ts (1)
93-106: Consider adjusting publintlevelto filter message types at source rather than in code.With
level: "suggestion", publint returns suggestions, warnings, and errors (theleveloption sets a minimum threshold). Since any non-empty messages array triggersprocess.exit(1), even benign suggestions fail the build. If this strictness is intentional, that's fine — but if suggestions become noise, the simplest fix is to uselevel: "warning"instead, which filters out suggestions at the API level rather than through post-processing. This avoids the asymmetry of logging suggestions but only exiting on errors/warnings.
| async load(id) { | ||
| if (!id.endsWith(SUFFIX)) return; | ||
|
|
||
| const filePath = id.slice(0, -SUFFIX.length); | ||
|
|
||
| const result = await build({ | ||
| entryPoints: [filePath], | ||
| bundle: true, | ||
| write: false, | ||
| format: "esm", | ||
| target: "esnext", | ||
| }); | ||
|
|
||
| const compiled = result.outputFiles[0].text; | ||
|
|
||
| return [ | ||
| `const code = ${JSON.stringify(compiled)};`, | ||
| `const blob = new Blob([code], { type: "application/javascript" });`, | ||
| `export default URL.createObjectURL(blob);`, | ||
| ].join("\n"); | ||
| }, |
There was a problem hiding this comment.
Missing this.addWatchFile — worklet source changes won't trigger rebuilds in dev mode.
Since the worklet file is resolved to a virtual module (id + SUFFIX), Vite doesn't track the original source file for changes. In watch/dev mode, edits to the worklet won't trigger a rebuild.
Proposed fix
async load(id) {
if (!id.endsWith(SUFFIX)) return;
const filePath = id.slice(0, -SUFFIX.length);
+ this.addWatchFile(filePath);
const result = await build({📝 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.
| async load(id) { | |
| if (!id.endsWith(SUFFIX)) return; | |
| const filePath = id.slice(0, -SUFFIX.length); | |
| const result = await build({ | |
| entryPoints: [filePath], | |
| bundle: true, | |
| write: false, | |
| format: "esm", | |
| target: "esnext", | |
| }); | |
| const compiled = result.outputFiles[0].text; | |
| return [ | |
| `const code = ${JSON.stringify(compiled)};`, | |
| `const blob = new Blob([code], { type: "application/javascript" });`, | |
| `export default URL.createObjectURL(blob);`, | |
| ].join("\n"); | |
| }, | |
| async load(id) { | |
| if (!id.endsWith(SUFFIX)) return; | |
| const filePath = id.slice(0, -SUFFIX.length); | |
| this.addWatchFile(filePath); | |
| const result = await build({ | |
| entryPoints: [filePath], | |
| bundle: true, | |
| write: false, | |
| format: "esm", | |
| target: "esnext", | |
| }); | |
| const compiled = result.outputFiles[0].text; | |
| return [ | |
| `const code = ${JSON.stringify(compiled)};`, | |
| `const blob = new Blob([code], { type: "application/javascript" });`, | |
| `export default URL.createObjectURL(blob);`, | |
| ].join("\n"); | |
| }, |
🤖 Prompt for AI Agents
In `@js/common/vite-plugin-worklet.ts` around lines 30 - 50, The load(id) handler
in vite-plugin-worklet.ts doesn’t register the original worklet source with
Vite’s watcher, so edits won’t trigger rebuilds; inside the async load(id)
function (where filePath is derived from id.slice(0, -SUFFIX.length)) call
this.addWatchFile(filePath) (guarded by checking this.addWatchFile exists)
before invoking build so Vite tracks the original file in dev/watch mode and
rebuilds the virtual module when the source changes.
- Include worklet.d.ts in hang-demo tsconfig so ?worklet imports resolve - Add addWatchFile in vite-plugin-worklet so edits trigger rebuilds - Change publint level from "suggestion" to "warning" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| if (val.endsWith(".css")) { | ||
| // CSS exports are only needed for dev-time resolution; | ||
| // consumers inline them at build time via @import. | ||
| // We purposely do not copy them to the dist to help catch bugs. |
There was a problem hiding this comment.
Eventually, we should roll them up and serve them out of public instead of inline.
|
|
||
| // Lint the package to catch publishing issues | ||
| console.log("🔍 Running publint..."); | ||
| const { messages, pkg: lintPkg } = await publint({ |
There was a problem hiding this comment.
Catches bugs with the generated package.json file. ex. It was pointing to non-existent css files.
| * then inlined as a string. At runtime, a blob URL is created and exported. | ||
| * Pass the URL to audioWorklet.addModule(). | ||
| */ | ||
| export function workletInline(): Plugin { |
There was a problem hiding this comment.
Eventually, we should serve this out of public instead.
| "rootDir": ".", | ||
| "outDir": "dist" | ||
| "rootDir": "./src", | ||
| "jsx": "preserve", |
There was a problem hiding this comment.
I didn't mean to make this change (claude pls) but I guess it's fine to have these JSX settings for the entire module.
… plugin Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Uh oh!
There was an error while loading. Please reload this page.