Skip to content

fix: Quote non-identifier object property keys - #2952

Open
cieplypolar wants to merge 1 commit into
feat/computed-propertiesfrom
fix/tseynit-quotes-around-non-identifiers
Open

fix: Quote non-identifier object property keys#2952
cieplypolar wants to merge 1 commit into
feat/computed-propertiesfrom
fix/tseynit-quotes-around-non-identifiers

Conversation

@cieplypolar

@cieplypolar cieplypolar commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

JS identifier rules
image
No need to worry about UnicodeEscapeSequence since it is evaluated by parser.

Copilot AI lite review requested due to automatic review settings August 31, 2026 19:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cieplypolar
cieplypolar changed the base branch from main to feat/computed-properties August 31, 2026 19:20
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown

pkg.pr.new

packages
Ready to be installed by your favorite package manager ⬇️

https://pkg.pr.new/software-mansion/TypeGPU/eslint-plugin-typegpu@c8aac6d99a023dc227f046384840526993190294
https://pkg.pr.new/software-mansion/TypeGPU/tinyest@c8aac6d99a023dc227f046384840526993190294
https://pkg.pr.new/software-mansion/TypeGPU/tinyest-for-wgsl@c8aac6d99a023dc227f046384840526993190294
https://pkg.pr.new/software-mansion/TypeGPU/typegpu@c8aac6d99a023dc227f046384840526993190294
https://pkg.pr.new/software-mansion/TypeGPU/@typegpu/gl@c8aac6d99a023dc227f046384840526993190294
https://pkg.pr.new/software-mansion/TypeGPU/unplugin-typegpu@c8aac6d99a023dc227f046384840526993190294

benchmark
view benchmark

commit
view commit

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown

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.76, 1.59, 3.33, 5.11, 6.33, 11.24, 21.52, 22.44]
  line [0.83, 1.70, 3.68, 5.56, 5.89, 9.69, 21.00, 21.58]
  line [0.72, 1.60, 3.24, 5.15, 6.22, 9.94, 20.29, 18.89]
Loading
---
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.31, 0.67, 0.64, 0.71, 0.93, 0.99, 1.14, 1.32]
  line [0.24, 0.46, 0.64, 0.76, 1.05, 1.09, 1.25, 1.41]
  line [0.24, 0.46, 0.66, 0.77, 1.01, 1.02, 1.28, 1.30]
Loading
---
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.81, 2.11, 2.91, 5.83, 10.97, 22.67, 48.37, 98.29]
  line [0.75, 2.02, 2.85, 6.43, 10.13, 22.13, 48.46, 97.34]
  line [0.90, 2.05, 3.06, 7.30, 10.76, 22.79, 47.97, 96.01]
Loading

@github-actions

Copy link
Copy Markdown

Bundle size comparison (import * as ... in PR vs import * as ... in target):

🟢 Decreased ➖ Unchanged 🔴 Increased (max 0.68%) ❔ Unknown
0 319 6 0

import * as ... in PR vs import * as ... in target (did bundle size increase?):

Test tsdown
tgpu_resolveWithContext.ts 162.99 kB ($${\color{red}+0.7\%}$$)
tgpu_resolve.ts 163.05 kB ($${\color{red}+0.7\%}$$)

If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.

@cieplypolar
cieplypolar force-pushed the fix/tseynit-quotes-around-non-identifiers branch from d563225 to 7b08363 Compare August 31, 2026 19:23

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The newly added quotes non-identifier object property keys test fails out of the box — both expect(...).toBe(...) arguments wrap the expected string in literal leading/trailing " characters that stringifyNode never produces, so the Test unit on source CI check is red (AssertionError: expected '{ "foo-bar": x, "with space": y }' to be '"{ "foo-bar": x, "with space": y }"').

Reviewed changes — full diff this run (2 files, 1 commit):

  • src/shared/tseynit.ts — routes object property keys through a new stringifyObjectPropertyKey(key) helper on both the objectExpr (Record form) and non-computed ObjectProperty paths, quoting any key that isn't a valid JS identifier via JSON.stringify.
  • tests/internal/tseynit.test.ts — adds a test covering dashed and space-containing keys on both object forms plus a computed string-literal key.

The implementation itself is sound: JS_IDENTIFIER faithfully mirrors the ECMAScript IdentifierName grammar (UnicodeIDStart + $/_, with ZWNJ/ZWJ only in continuation), JSON.stringify handles quote/backslash/control-char escaping, identifier keys keep their previous output so existing snapshots are unaffected (confirmed — the rest of the suite passes), and every stringifyNode consumer is an error-message renderer in wgslGenerator.ts, so the produced string is never executed. The only problem is the two broken assertions, which block merge until corrected.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread packages/typegpu/tests/internal/tseynit.test.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — incremental delta on top of the prior review (commit d563225907b08363):

  • tests/internal/tseynit.test.ts — the two expect(...).toBe(...) calls in the new test drop the spurious leading/trailing " wrappers flagged in the previous review. I re-ran the file locally: all 31 tests pass.

The implementation and test logic reviewed earlier are unchanged and sound, and this commit resolves the one blocking finding.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@cieplypolar
cieplypolar force-pushed the fix/tseynit-quotes-around-non-identifiers branch from 7b08363 to 6830a02 Compare September 1, 2026 10:44
@cieplypolar
cieplypolar force-pushed the fix/tseynit-quotes-around-non-identifiers branch from 6830a02 to 6e459f3 Compare September 1, 2026 11:19

@aleksanderkatan aleksanderkatan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@cieplypolar
cieplypolar force-pushed the fix/tseynit-quotes-around-non-identifiers branch from 6e459f3 to b613358 Compare September 4, 2026 15:36
@cieplypolar
cieplypolar force-pushed the fix/tseynit-quotes-around-non-identifiers branch from b613358 to c8aac6d Compare September 4, 2026 15:40
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.

3 participants