Repository navigation
Conversation
|
pkg.pr.new packages benchmark commit |
77eb7a9 to
d0d1927
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Nominal comparison accepts distinct, incompatible struct schemas, undermining accessor validation.
0 open findings
What changed in this PR
Introduces explicit structural and nominal schema comparisons in TypeGPU, with nominal matching for accessor validation.
Changes:
- Adds both comparison APIs and retains
deepEqualas a deprecated alias. - Migrates serialization tests and React schema stabilization to structural comparison.
- Adds nominal identity tests and renames existing comparison tests.
| File | Description |
|---|---|
| packages/typegpu/tests/serializeDataSchema.test.ts | Uses structural comparison for round trips. |
| packages/typegpu/tests/serial.test.ts | Updates resource schema comparisons. |
| packages/typegpu/tests/internal/schemasEqualStructurally.test.ts | Checks structural pointer comparison. |
| packages/typegpu/tests/internal/schemasEqualNominally.test.ts | Checks nominal pointer comparison. |
| packages/typegpu/tests/internal/deepEqual.test.ts | Removes replaced pointer tests. |
| packages/typegpu/tests/data/schemasEqualStructurally.test.ts | Preserves structural comparison coverage. |
| packages/typegpu/tests/data/schemasEqualNominally.test.ts | Adds nominal identity coverage. |
| packages/typegpu/tests/data/deepEqual.test.ts | Removes replaced structural tests. |
| packages/typegpu/src/data/schemasEqual.ts | Implements both comparison modes. |
| packages/typegpu/src/data/index.ts | Exports APIs and deprecated alias. |
| packages/typegpu/src/core/slot/accessor.ts | Switches validation to nominal comparison. |
| packages/typegpu-react/src/core/helper-hooks.ts | Uses structural schema stabilization. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased (max 2.37%) | ❔ Unknown |
|---|---|---|---|
| 0 | 343 | 6 | 2 |
import * as ... in PR vs import * as ... in target (did bundle size increase?):
| Test | tsdown |
|---|---|
| d_deepEqual.ts | 2.62 kB ( |
| d_schemasEqualNominally.ts | 2.62 kB |
| d_schemasEqualStructurally.ts | 2.62 kB |
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
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 [0.72, 1.45, 3.10, 5.10, 6.09, 9.61, 19.14, 17.43]
line [0.65, 1.40, 3.16, 5.04, 6.01, 8.61, 18.04, 18.93]
line [0.72, 1.40, 2.97, 4.92, 5.58, 9.82, 18.28, 20.56]
---
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.24, 0.41, 0.53, 0.61, 0.80, 0.86, 1.00, 1.18]
line [0.23, 0.38, 0.52, 0.61, 0.82, 0.86, 1.00, 1.10]
line [0.27, 0.41, 0.55, 0.65, 0.85, 0.86, 1.03, 1.14]
---
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 [0.59, 1.57, 2.97, 5.56, 9.55, 19.11, 41.14, 84.83]
line [0.66, 1.63, 3.06, 5.56, 9.48, 19.40, 41.41, 85.73]
line [0.72, 1.68, 2.94, 5.13, 9.62, 19.75, 42.23, 85.41]
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
schemasEqualNominally does not compare structs/unstructs at all: it reports two distinct structs as equal. The PR's own nominal suite fails 8 assertions, and pnpm test:style also fails on schemasEqual.ts.
Reviewed changes
Reviewed the full diff introducing schemasEqualStructurally/schemasEqualNominally and the call-site migrations.
- New comparison API —
packages/typegpu/src/data/deepEqual.tsbecomesschemasEqual.ts, with structural/nominal exports over a sharedschemasEqual(a, b, structural);deepEqualis kept as a deprecated alias of the structural variant. - Call-site migration — react's
useStableSchemaand the serial/serialize tests use the structural function;packages/typegpu/src/core/slot/accessor.tsmatchesSchemauses the nominal one. - Test reorganization — the old
deepEqualsuites are removed and split into structural/nominal suites (plus internal implicit-pointer tests).
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ The new commit is a formatting pass only; it resolves the
oxfmtnit from the previous review. The remaining findings there are still open.
Reviewed changes
Re-reviewed only the delta between 77eb7a9 and d0d1927.
- Formatting —
oxfmtapplied acrossschemasEqual.ts, the two new comparison suites,serial.test.ts, andserializeDataSchema.test.ts; the extra blank line between the two exports is gone, andnpx oxfmt --checknow passes on those files. - No logic changes —
packages/typegpu/src/data/schemasEqual.ts:58still gates the struct branch onstructural, so the 8schemasEqualNominallyfailures reproduced against the previous commit still reproduce here (npx vitest run --project=!browser packages/typegpu/tests/data/schemasEqualNominally.test.ts→ 8 failed).
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
c47790e to
02acc88
Compare
There was a problem hiding this comment.
Caution
The nominal fix activates a regression at the matchesSchema call site. Now that schemasEqualNominally actually compares structs by identity, two existing accessor tests fail: pnpm test:unit reports 2 failed / 2918 passed (tests/accessor.test.ts > can provide parts of a bind group layout and > can provide a runtime-sized array).
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (c47790e → 02acc88), which is confined to packages/typegpu/src/data/schemasEqual.ts.
- Fixed the nominal struct skip — the
isWgslStruct/isUnstructbranch is no longer gated onstructural; in nominal mode it now returnsa === b, so two distinct struct instances are no longer reported equal. The 8 previously-failingschemasEqualNominallyassertions now pass. - Doc reflow — the
@example/import block in theschemasEqualNominallydoc comment was moved.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
02acc88 to
75e4e70
Compare
75e4e70 to
4fe1864
Compare
4fe1864 to
ea51187
Compare
There was a problem hiding this comment.
ℹ️ No critical issues — the accessor regression is fixed. One description mismatch noted inline.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (4fe1864 → ea51187), which touches only packages/typegpu/src/core/slot/accessor.ts.
- Accessor switched back to structural comparison —
matchesSchemausesschemasEqualStructurallyinstead ofschemasEqualNominally, restoring the historical acceptance of structurally-equal but distinct structs. The two previously failing accessor tests now pass (verified: accessor + both equality suites, 88 passed). - No other authored delta — the branch was squashed/force-pushed, but the full diff still contains just the schema-equality API, call-site migrations, and test reorganization.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
ea51187 to
0cd0729
Compare
There was a problem hiding this comment.
ℹ️ No new issues — the delta is a behavior-neutral clarity fix that addresses prior feedback.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (ea51187 → 0cd0729), which touches only packages/typegpu/src/data/schemasEqual.ts.
- Explicit nominal struct return — the
!structuralstruct branch now returnsfalseinstead ofa === b. Identity is already handled by thea === bearly return at the top ofschemasEqual, so the two are equivalent; the change makes the intent explicit and the inline comment was reworded to read correctly. - JSDoc reflow — the
@param/@returnsblock in theschemasEqualNominallydoc comment was repositioned.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
No description provided.