Modernize @tko/utils: drop dead polyfill probes#344
Conversation
Remove legacy compat shims that pre-date ES2015+ as baseline: - symbol.ts: useSymbols probe is always true; inline Symbol() call - function.ts: functionSupportsLengthOverwrite probe is always true; Function.length has been configurable since ES6 - string.ts: stringTrim no longer needs the \xa0-regex fallback; stringStartsWith delegates to native String.prototype.startsWith - css.ts: every Element has classList; drop SVGAnimatedString and string-className fallbacks - preparse.ts: stringTrim's tightened return type (string, not any) surfaced a null gap where str.match() could return null; default to [] at both call sites Public API (createSymbolOrString, overwriteLengthPropertyIfSupported, stringTrim, stringStartsWith, parseJson, toggleDomNodeCssClass) unchanged. Internal probe exports (useSymbols, functionSupportsLengthOverwrite) removed — they had no external consumers in the monorepo. Net -40 lines. All 2698 browser tests and 2671 happy-dom tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 28 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe pull request modernizes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
The wrapper was only there to gate on the dead `functionSupportsLengthOverwrite` probe. With the probe gone the function is a trivial alias — call sites in @tko/observable now invoke `Object.defineProperty(…, 'length', descriptor)` directly. packages/utils/src/function.ts removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49576cbae3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| export function createSymbolOrString(identifier) { | ||
| return useSymbols ? Symbol(identifier) : identifier | ||
| export function createSymbolOrString(identifier: string): symbol { |
There was a problem hiding this comment.
useSymbols as a compatibility export
@tko/utils publicly re-exports src/symbol.ts, so removing useSymbols is a breaking API change for downstream code that imports it (import { useSymbols } from '@tko/utils'). In this commit the symbol helper is narrowed to always return Symbol, but the removed named export will still break consumers immediately in both TS and runtime module resolution while this is marked as a patch release.
Useful? React with 👍 / 👎.
| if (functionSupportsLengthOverwrite) { | ||
| Object.defineProperty(fn, 'length', descriptor) | ||
| } | ||
| export function overwriteLengthPropertyIfSupported(fn: Function, descriptor: PropertyDescriptor): void { |
There was a problem hiding this comment.
functionSupportsLengthOverwrite export
Because @tko/utils re-exports src/function.ts, deleting functionSupportsLengthOverwrite removes a previously available named export and creates a semver-breaking change for any consumer importing it from the package root. Even if the runtime probe is now unnecessary, retaining an exported compatibility constant (or shipping a major bump) avoids unexpected build/runtime failures for existing users.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Phase A of @tko/utils modernization: removes legacy runtime feature-detection branches and fallback code paths that are no longer needed for supported runtimes, keeping the runtime behavior effectively the same while simplifying implementations.
Changes:
- Simplifies
@tko/utilshelpers by removing polyfill probes and legacy fallbacks (Symbols, string trim/startsWith, function length overwrite, DOM class toggling). - Tightens some utility implementations and TypeScript annotations.
- Makes
@tko/utils.parserobject-literal tokenization resilient toString.prototype.matchreturningnull.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/utils/src/symbol.ts | Removes useSymbols probe; createSymbolOrString now always returns a Symbol with a stricter TS signature. |
| packages/utils/src/string.ts | Removes trim/startsWith fallbacks; uses native trim()/startsWith() with updated TS signatures. |
| packages/utils/src/function.ts | Removes length-overwrite feature probe; always calls Object.defineProperty with typed params. |
| packages/utils/src/css.ts | Drops SVG/className fallback paths; relies on classList and token iteration. |
| packages/utils.parser/src/preparse.ts | Guards match() results with ?? [] to avoid null handling issues. |
| .changeset/modernize-utils-dead-polyfills.md | Adds changeset marking @tko/utils and @tko/utils.parser as patch bumps for this modernization. |
Comments suppressed due to low confidence (1)
packages/utils/src/function.ts:2
overwriteLengthPropertyIfSupportedis a public export and this commit changes its TS signature from implicitanyparameters tofn: Functionanddescriptor: PropertyDescriptor. That can be a breaking typing change for downstream TS callers (especially those passing a callable type rather thanFunction, or building descriptors dynamically). If the intent is “no API change” in a patch, consider keeping the parameters more permissive (or using a more precise callable type thanFunction) to reduce downstream breakage.
| return false | ||
| } | ||
| return string.substring(0, startsWith.length) === startsWith | ||
| export function stringStartsWith(value: string | null | undefined, prefix: string): boolean { |
|
|
||
| export function createSymbolOrString(identifier) { | ||
| return useSymbols ? Symbol(identifier) : identifier | ||
| export function createSymbolOrString(identifier: string): symbol { |
| @@ -0,0 +1,27 @@ | |||
| --- | |||
| "@tko/utils": patch | |||
- `createSymbolOrString(id)` removed; 7 call sites across binding.core,
binding.foreach, computed, lifecycle, and builder now call `Symbol(id)`
directly. `ko.utils.createSymbolOrString` is no longer on the public API
(minor bump for @tko/utils, @tko/builder).
- `stringTrim(v)` and `stringStartsWith(v, p)` removed; 3 call sites across
binding.core and utils.parser use `String(v ?? '').trim()` and
`v.startsWith(p)` inline.
- `builds/knockout/spec/bindingAttributeBehaviors.js` — drop dead branch
gated on `createSymbolOrString('') === ''` (always false with real Symbol).
- `packages/utils/src/{function,symbol}.ts` deleted.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- `tasks.ts`: collapse multi-line condition; use optional chaining on `schedulerGlobal?.MutationObserver` and `schedulerGlobal.navigator?.standalone` - `foreach.ts`: single-line import formatting Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Phase A of
@tko/utilsmodernization. Removes runtime feature detection for capabilities every supported runtime (modern browsers, Node, Bun, happy-dom) already provides unconditionally. No behavior change — the guards were alltruebranches in practice.Dropped
functionSupportsLengthOverwriteprobe —Object.defineProperty(fn, 'length', …)has worked since IE9useSymbolsprobe —Symbolis always definedstringTrimpolyfill fallback — always usesString.prototype.trimstringStartsWithmanualsubstringcomparison — delegates toString.prototype.startsWithtoggleDomNodeCssClassSVGAnimatedString fallback +toggleObjectClassPropertyStringhelper —classListworks on every supportedElement(SVG2)parseJsonindirection throughstringTrimOther
packages/utils.parser/src/preparse.tsguardsstr.match(bindingToken) ?? []— previously relied on the transformed input never producing anullmatch. Paranoia, not a known bug.Verification
bun run build— cleanbunx vitest run --project browser— 2708 pass / 42 skipbunx vitest run --project cli-happy-dom— 2681 pass / 69 skipbunx tsc --noEmit— cleanTest plan
Refs #335 — tracking plan for @tko/utils modernization.
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
classListAPI exclusively.Bug Fixes