Conversation
|
pkg.pr.new packages benchmark commit |
Resolution Time Benchmark---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Random Branching (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [1.02, 2.06, 4.92, 6.99, 8.12, 12.91, 26.30, 27.90]
line [1.01, 2.14, 4.99, 6.92, 8.25, 12.97, 26.45, 26.43]
line [1.07, 2.14, 4.37, 7.26, 8.70, 13.82, 25.24, 25.55]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Linear Recursion (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.34, 0.56, 0.69, 0.84, 1.16, 1.26, 1.45, 1.64]
line [0.30, 0.52, 0.74, 0.90, 1.24, 1.32, 1.51, 1.70]
line [0.30, 0.52, 0.72, 0.90, 1.19, 1.25, 1.53, 1.69]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Full Tree (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [1.00, 2.27, 4.35, 7.32, 12.97, 27.82, 59.92, 121.71]
line [0.85, 2.35, 4.35, 7.90, 13.35, 28.60, 61.12, 122.09]
line [0.95, 2.48, 3.98, 7.60, 13.38, 27.80, 59.37, 122.49]
|
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased | ❔ Unknown |
|---|---|---|---|
| 0 | 325 | 0 | 0 |
No notable changes.
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
c03493b to
2ce9dfa
Compare
2ce9dfa to
ee0c709
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The Node hook currently derives non-file module IDs from URL.pathname (percent-encoded and origin-less), which can cause incorrect filtering/debug output for remote modules and should be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Node.js/Deno synchronous loader-hook support to unplugin-typegpu, enabling runtime (non-bundler) shader transformation via Node’s registerHooks-based module hooks and a Deno alias.
Changes:
- Added new
unplugin-typegpu/nodeinstaller andunplugin-typegpu/denoalias entrypoints for synchronous module hooks. - Added unit + integration tests covering hook behavior across extensions and runtime modes.
- Updated package exports/build inputs and expanded README/docs to document Node/Deno (and Bun preload) usage.
File summaries
| File | Description |
|---|---|
| packages/unplugin-typegpu/tsdown.config.ts | Includes new Node/Deno entrypoints in the build inputs. |
| packages/unplugin-typegpu/test/node.test.ts | Unit tests for the hook installer behavior and transformation rules. |
| packages/unplugin-typegpu/test/node-integration.test.ts | Integration tests that exercise runtime execution under Node/Deno. |
| packages/unplugin-typegpu/src/node.ts | Implements synchronous registerHooks-based loader hook installer. |
| packages/unplugin-typegpu/src/deno.ts | Deno alias entrypoint re-exporting the Node installer. |
| packages/unplugin-typegpu/src/core/filter.ts | Updates comment about where custom filtering is used. |
| packages/unplugin-typegpu/README.md | Documents runtime hooks + adjusts Bun preload example. |
| packages/unplugin-typegpu/package.json | Adds Node/Deno exports entries (source + built). |
| apps/typegpu-docs/src/content/docs/tooling/unplugin-typegpu.mdx | Adds Node/Deno runtime hook documentation section. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const moduleURL = new URL(url); | ||
| const id = moduleURL.protocol === 'file:' ? fileURLToPath(moduleURL) : moduleURL.pathname; |
| @@ -1,5 +1,5 @@ | |||
| // Copied from https://github.com/unjs/unplugin/blob/f514bf8e2d751b48b3a5acb1077eb1292d9711b3/src/utils/filter.ts#L2 | |||
| // Used only in the babel version of the plugin, since others can rely on native unplugin functionality. | |||
| // Used by the Babel and Deno integrations, which cannot rely on native unplugin filtering. | |||
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions only — the implementation is sound: typecheck clean, all 161 tests pass (including both real-Node integration legs), the build emits correct ESM/CJS shims, and the
registerHookshook shape matches the official Node/Deno docs. Two things worth a look below.
Reviewed changes
unplugin-typegpu/nodeinstaller (src/node.ts) —install()registers a synchronousloadhook vianode:moduleregisterHooks, reusingunpluginFactory,createFilterForId, andearlyPruneRegex, with binary-source decoding and deregistration passthrough.unplugin-typegpu/denoalias (src/deno.ts) — thin re-export of the node installer;package.jsonexports/publishConfig andtsdown.config.tsextended for both entries.- Tests —
node.test.tsunit-tests the hook (extensions, binary sources, remote URLs, globs, options, deregister);node-integration.test.tsspawns realnode/denosubprocesses in dynamic-import and--importpreload modes. - Docs — "Node.js and Deno" sections in the docs site and README, plus a Bun preload/
bunfig.toml/Bun.buildcleanup.
ℹ️ Deno integration never runs in CI
No workflow installs Deno (.github/workflows/*.yml only set up Node 24.x), so supportsHooks is always false on CI and the Deno leg of node-integration.test.ts — the primary platform this PR adds support for — is silently skipped. node.test.ts mocks node:module, so it cannot close this gap either. As written, a regression in Deno's node:module compat or hook behavior would ship with zero CI signal.
Technical details
# Add Deno to the CI gate for the loader integration tests
## Affected sites
- `.github/workflows/test-unit.yml` — runs the package tests via `pnpm test:unit-and-attest`; add a `denoland/setup-deno` step (with a version satisfying `>= 2.8`) next to the existing `actions/setup-node@v4` step.
- `packages/unplugin-typegpu/test/node-integration.test.ts:8-11` — the `supportsHooks` version gate; with Deno on the runner both `describe.each(['node', 'deno'])` legs execute.
## Required outcome
- The Deno leg (dynamic import + preload modes, `.jsx`/`.tsx`) actually executes in CI instead of skipping.
## Open questions for the human
- Should a missing/too-old Deno fail the job loudly under CI (e.g. `expect(supportsHooks).toBe(true)` when `process.env.CI`), or stay a silent skip so older local setups can still run the suite?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| const moduleURL = new URL(url); | ||
| const id = moduleURL.protocol === 'file:' ? fileURLToPath(moduleURL) : moduleURL.pathname; |
There was a problem hiding this comment.
Non-file module ids come from moduleURL.pathname, which stays percent-encoded (verified on Node 24: https://host/my%20dir/shader.ts → id /my%20dir/shader.ts), while file: ids are percent-decoded by fileURLToPath. The default include regex is suffix-anchored so both work, but a user glob in the **/my shaders/*.ts shape (already unit-tested for file: URLs) would silently miss a matching remote module, and error/debug output would show the encoded path. Decoding the pathname keeps non-file ids consistent with the file: form.
| const moduleURL = new URL(url); | |
| const id = moduleURL.protocol === 'file:' ? fileURLToPath(moduleURL) : moduleURL.pathname; | |
| const id = moduleURL.protocol === 'file:' ? fileURLToPath(moduleURL) : decodeURIComponent(moduleURL.pathname); |
| } | ||
|
|
||
| const transformed = plugin.transform.handler.call(transformContext, code, id); | ||
| return transformed ? { ...result, source: transformed.code } : result; |
There was a problem hiding this comment.
As we discussed in person, source maps are discarded. They live on transformed.map property. You can verify this yourself
import install from './src/node.ts';
if (!import.meta.url.endsWith('?transformed')) {
install();
await import(`${import.meta.url}?transformed`);
} else {
const shader = () => {
'use gpu';
return 42;
};
throw new Error('This error should point to line 12');
}The good news is there is an easy fix (by my beloved clanker Astra)
| return transformed ? { ...result, source: transformed.code } : result; | |
| return transformed ? { ...result, source: `${transformed.code}\n//# sourceMappingURL=${transformed.map.toUrl()}` } : result; |
This aligns with the approach described here:
https://medium.com/the-node-js-collection/source-maps-in-node-js-5d0abe6ac34b#:~:text=/-,/%23%20sourceMappingURL%3Dtest.js.map,-This%20special%20comment
This PR adds the
unplugin-typegpu/nodeandunplugin-typegpu/denoentrypoints that expose a hook config that can be used by theregisterHooksNode API.