Skip to content

convert inline worker generation to a post-processing step#449

Open
Devin T. Currie (DTCurrie) wants to merge 18 commits into
mainfrom
make-worker-transform-post-process
Open

convert inline worker generation to a post-processing step#449
Devin T. Currie (DTCurrie) wants to merge 18 commits into
mainfrom
make-worker-transform-post-process

Conversation

@DTCurrie

@DTCurrie Devin T. Currie (DTCurrie) commented Mar 3, 2026

Copy link
Copy Markdown
Member

Web workers using new Worker(new URL('./worker.js', import.meta.url)) break when consumed as a library because Vite's dep optimizer moves the package to .vite/deps/ but the worker file doesn't follow.

This PR switches from pre-generating inlined worker files in src/lib to post-processing dist/ after svelte-package.

  • src/lib/loaders/pcd/index.ts reverted to standard new Worker(new URL(...)) syntax
  • scripts/post-process-workers.js runs after svelte-package, bundles workers with esbuild, and rewrites dist/ to use Blob URLs
  • check/prepare scripts no longer need a pre-build step
  • Added scripts/README.md documenting both build scripts

The previous approach (pre-generating worker.inline.ts before svelte-package) required running a build step before check, didn't support HMR for worker changes in dev, and left generated files in the source tree. Post-processing dist/ solves all three: dev mode works natively with Vite, CI runs check without a pre-build, and the source stays clean.

I tested running locally, and the changes were reflected in HMR. By running pnpm preview and make up in the environments, I was able to file-drop a PCD and load it.

  • Test downstream with linking

@changeset-bot

changeset-bot Bot commented Mar 3, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: d9aa21f

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

@codecov

codecov Bot commented Mar 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 1.56250% with 63 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
scripts/post-process-workers.js 0.00% 62 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://viamrobotics.github.io/visualization/pr-preview/pr-449/

Built to branch gh-pages at 2026-07-08 20:54 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@DTCurrie

Copy link
Copy Markdown
Member Author

Claude (@claude) I just updated this PR after it was stale for a while. Review the changes, ensure they are correct, and confirm that no other changes to the codebase since this was written will affect this PR.

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished Devin T. Currie (@DTCurrie)'s task in 3m 35s —— View job


PR Review: convert inline worker generation to a post-processing step

  • Read CLAUDE.md and gather context
  • Run git diff origin/main...HEAD to see full PR changes
  • Read all modified files in full
  • Check for codebase changes since PR was written that may affect it
  • Review for correctness, security, and conventions
  • Post final review

Summary

The approach is sound — post-processing dist/ to inline workers avoids the Vite dep-optimizer bug and cleans up the source tree. Dev mode works correctly with native Vite worker handling. However, there is a critical bug in the wireit configuration that means the post-processing never actually runs when publishing the package.


Critical

build:workers runs before dist/ exists — workers are never inlined (see inline comment)

post-process-workers.js is wired as a dependency of the package step, which means wireit runs it before svelte-package creates dist/. The script calls globSync('dist/**/*.js'), finds nothing, and exits silently. The published package still contains raw new Worker(new URL(...)) calls — the original bug this PR is trying to fix remains unfixed.

The README describes the correct flow: svelte-package && node scripts/post-process-workers.js && publint. The wireit config needs to match it:

  1. Move the post-processing into the package command: "svelte-package && node scripts/post-process-workers.js && publint".
  2. Remove build:workers from package's dependencies (it no longer generates any source file needed by svelte-package).
  3. Fix build:workers wireit metadata: files still lists scripts/build-workers.js (deleted) and output still lists src/lib/loaders/pcd/worker.inline.ts (deleted) — wireit's incremental caching will be broken.
  4. Remove the now-unnecessary build:workers dependency from vite-build and check (it used to generate worker.inline.ts that was imported; the new index.ts doesn't need any pre-generated file).
  5. Remove pnpm build:workers && from check:watch (line 22) and prepare (line 18) — both are no-ops in the new setup.

Minor

Regex fragility in post-process-workers.js (see inline comment)

  • \{[^}]*\} won't match nested braces in the options object (fine for now, fragile for future additions).
  • .replace(fullMatch, replacement) on line 64 only replaces the first occurrence. If a file contains the same worker pattern twice, only the first is rewritten. .replaceAll is safer.

Missing ./ in worker path (see inline comment)

new URL('worker.js', import.meta.url) — bare name vs './worker.js'. Relative paths with ./ are more idiomatic for sibling files and avoid any ambiguity with module resolution.


Codebase changes since PR was written

The PR has been rebased/merged against main multiple times (most recently at 2764dac). The only main-branch change that touches relevant territory is the addition of the add point cloud chunking commit (79e18f2) which modified the PCD loader. Since this PR reverts src/lib/loaders/pcd/index.ts to the simpler worker form, that change is already incorporated in the current state of the file. No other recent main changes conflict with this PR.


The PR is blocked on the wireit ordering fix — without it, pnpm prepack will publish a broken package. Everything else is minor.

Comment thread package.json
Comment thread scripts/post-process-workers.js
Comment thread src/lib/loaders/pcd/index.ts
@DTCurrie

Copy link
Copy Markdown
Member Author

Summary

The approach is sound — post-processing dist/ to inline workers avoids the Vite dep-optimizer bug and cleans up the source tree. Dev mode works correctly with native Vite worker handling. However, there is a critical bug in the wireit configuration that means the post-processing never actually runs when publishing the package.

Critical

build:workers runs before dist/ exists — workers are never inlined (see inline comment)

post-process-workers.js is wired as a dependency of the package step, which means wireit runs it before svelte-package creates dist/. The script calls globSync('dist/**/*.js'), finds nothing, and exits silently. The published package still contains raw new Worker(new URL(...)) calls — the original bug this PR is trying to fix remains unfixed.

The README describes the correct flow: svelte-package && node scripts/post-process-workers.js && publint. The wireit config needs to match it:

  1. Move the post-processing into the package command: "svelte-package && node scripts/post-process-workers.js && publint".
  2. Remove build:workers from package's dependencies (it no longer generates any source file needed by svelte-package).
  3. Fix build:workers wireit metadata: files still lists scripts/build-workers.js (deleted) and output still lists src/lib/loaders/pcd/worker.inline.ts (deleted) — wireit's incremental caching will be broken.
  4. Remove the now-unnecessary build:workers dependency from vite-build and check (it used to generate worker.inline.ts that was imported; the new index.ts doesn't need any pre-generated file).
  5. Remove pnpm build:workers && from check:watch (line 22) and prepare (line 18) — both are no-ops in the new setup.

Minor

Regex fragility in post-process-workers.js (see inline comment)

  • \{[^}]*\} won't match nested braces in the options object (fine for now, fragile for future additions).
  • .replace(fullMatch, replacement) on line 64 only replaces the first occurrence. If a file contains the same worker pattern twice, only the first is rewritten. .replaceAll is safer.

Missing ./ in worker path (see inline comment)

new URL('worker.js', import.meta.url) — bare name vs './worker.js'. Relative paths with ./ are more idiomatic for sibling files and avoid any ambiguity with module resolution.

Codebase changes since PR was written

The PR has been rebased/merged against main multiple times (most recently at 2764dac). The only main-branch change that touches relevant territory is the addition of the add point cloud chunking commit (79e18f2) which modified the PCD loader. Since this PR reverts src/lib/loaders/pcd/index.ts to the simpler worker form, that change is already incorporated in the current state of the file. No other recent main changes conflict with this PR.

The PR is blocked on the wireit ordering fix — without it, pnpm prepack will publish a broken package. Everything else is minor.

Claude (@claude) make these fixes

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished Devin T. Currie (@DTCurrie)'s task in 2s —— View job


Applied all review fixes

  • Read affected files
  • Fix package.json wireit config (critical — execution order)
  • Fix post-process-workers.js (use .replaceAll)
  • Fix src/lib/loaders/pcd/index.ts (./worker.js)
  • Run pnpm lint (prettier clean; eslint skipped — not in allowedTools) and pnpm check (0 errors, 0 warnings)
  • Commit and push

What was changed (commit 37dd415):

package.json

  • package wireit command: svelte-package && node scripts/post-process-workers.js && publint — post-processing now runs after svelte-package creates dist/, not before as a dependency
  • Removed build:workers from package, vite-build, and check wireit dependencies — index.ts no longer imports a pre-generated file, so there's no pre-build needed
  • Fixed build:workers wireit metadata: files now references scripts/post-process-workers.js (not the deleted build-workers.js), output is now [] (not the deleted worker.inline.ts)
  • prepare: removed && pnpm build:workers (no-op in the new setup)
  • check:watch: removed pnpm build:workers && (no-op in the new setup)

scripts/post-process-workers.js

  • Line 64: result.replace(...)result.replaceAll(...) so all occurrences of the same pattern in a file are rewritten, not just the first

src/lib/loaders/pcd/index.ts

  • 'worker.js''./worker.js' for idiomatic relative path resolution
    |

github-actions Bot and others added 4 commits July 8, 2026 15:50
- package command now runs svelte-package && node scripts/post-process-workers.js && publint
  so post-processing happens after dist/ is created, not before
- remove build:workers from package, vite-build, and check wireit dependencies
- fix build:workers wireit metadata: correct files reference and empty output array
- remove pnpm build:workers from prepare and check:watch scripts
- use replaceAll in post-process-workers.js to handle duplicate worker patterns
- use ./worker.js (relative path) in pcd/index.ts

Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants