Fix cloning errors for objects with custom JSON serialization#315
Closed
Fix cloning errors for objects with custom JSON serialization#315
Conversation
Co-authored-by: DanWahlin <1767249+DanWahlin@users.noreply.github.com>
Co-authored-by: DanWahlin <1767249+DanWahlin@users.noreply.github.com>
Co-authored-by: DanWahlin <1767249+DanWahlin@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix deep cloning process for complex objects
Fix cloning errors for objects with custom JSON serialization
Jan 21, 2026
DanWahlin
added a commit
that referenced
this pull request
Feb 6, 2026
Resolves #314 — ClonerService crashes with TypeError when state contains objects like Dayjs or Moment whose toJSON() returns primitives. Root cause: JSON.parse(JSON.stringify()) converts complex objects to primitives, then fixTypes() tries to set properties on them. Solution: Detect complex objects (custom prototypes with methods, or toJSON returning primitives) and clone them safely via: 1. clone() method if available (Dayjs, Moment, Luxon all have this) 2. Constructor with original value 3. Object.create with same prototype + property copy 4. Reference fallback (last resort) Key differences from Copilot PR #315: - Actually clones complex objects instead of sharing references - Prevents mutation footgun (shared refs let mutations leak) - Handles arrays containing complex objects - Handles deeply nested complex objects (3+ levels) - Recursive containsComplexValues catches them anywhere in tree 22 new tests covering all scenarios.
Owner
|
Superseded by #324, which takes a different approach: instead of preserving references to complex objects (which breaks mutation isolation — the core promise of a state management library), #324 actually clones them using a safe strategy chain ( See the comparison table in #324 for full details. The key difference: with #315, mutating a Dayjs object from 22 new tests cover Dayjs/Moment-like objects, arrays, mutation isolation, mixed types, and edge cases. All 59 specs pass. |
DanWahlin
added a commit
that referenced
this pull request
Feb 6, 2026
…rades (#324) * Fix deep cloning for complex objects (Dayjs, Moment, etc.) Resolves #314 — ClonerService crashes with TypeError when state contains objects like Dayjs or Moment whose toJSON() returns primitives. Root cause: JSON.parse(JSON.stringify()) converts complex objects to primitives, then fixTypes() tries to set properties on them. Solution: Detect complex objects (custom prototypes with methods, or toJSON returning primitives) and clone them safely via: 1. clone() method if available (Dayjs, Moment, Luxon all have this) 2. Constructor with original value 3. Object.create with same prototype + property copy 4. Reference fallback (last resort) Key differences from Copilot PR #315: - Actually clones complex objects instead of sharing references - Prevents mutation footgun (shared refs let mutations leak) - Handles arrays containing complex objects - Handles deeply nested complex objects (3+ levels) - Recursive containsComplexValues catches them anywhere in tree 22 new tests covering all scenarios. * Modernize dev dependencies - TypeScript 4.9.3 → 5.9.3 - Jasmine 3.9 → 6.0 (replaced jasmine-ts with native ts-node register) - RxJS 7.5.7 → 7.8.2 - ts-node 10.2.1 → 10.9.2 - @types/jasmine 3.x → 6.x - @types/node 18.x → 22.x - Removed jasmine-ts (unmaintained) - Build target es5 → es2018 - All 59 tests pass, library builds clean * Switch build and test tooling to Vite + Vitest Build: - Replaced tsc with Vite library mode (vite build) - Produces dual ESM (.js) + CJS (.cjs) output - Bundled type declarations via vite-plugin-dts - Added proper package.json exports map for ESM/CJS resolution - Target: ES2022 Tests: - Replaced Jasmine + jasmine-ts with Vitest - Migrated jasmine.createSpy() → vi.fn().mockImplementation() - Migrated .toBeTrue() → .toBe(true) - Removed spec/ directory (jasmine.json, ts-node-register, reporter) - All 59 tests pass (46ms execution, 652ms total with startup) Removed: - jasmine, jasmine-ts, jasmine-spec-reporter, @types/jasmine - ts-node, @types/node - spec/ helper directory Added: - vite, vitest, vite-plugin-dts * Code quality fixes (non-breaking) - Fix shallow Map/Set cloning: values are now deep-cloned (mutation isolation) - Replace hasOwnProperty() with Object.hasOwn() (ES2022, safer) - Remove unused rxjs imports from interfaces.ts (Subscription, Observable) - Fix var → const in fixPropertyValue - Fix loose equality (==) → strict (===) for Infinity check - Reduce redundant deep clones in setState() (3→2 getState calls; previousState clone deferred when trackStateHistory is off) - Remove stale ES2015 comment - Add 2 new tests: deep Map clone isolation, deep Set clone isolation - All 61 tests pass * Fix Vite build target for backward compat with Angular 15 webpack - Set build target to es2015 to ensure class syntax is transpiled for consumers that use older webpack-based build pipelines (Angular 15) - Add keepNames to preserve class names through minification - Add preserveSymlinks to Angular sample tsconfig for npm link testing The 'Class constructor cannot be invoked without new' error occurred because Angular 15's webpack wraps native ES2022 classes in a way that breaks the super() chain. es2015 target transpiles classes to constructor functions that webpack can handle. Note: The observable-store-extensions package also needs a Vite build update to resolve the same issue when ReduxDevToolsExtension is used. * Modernize observable-store-extensions to Vite + TS 5.9 - Switch from tsc to Vite library mode (dual ESM/CJS output) - TypeScript 4.9.3 → 5.9.3 - RxJS 7.5.7 → 7.8.2 - Added proper package.json exports map - Fixed bare 'interfaces' import → relative '../interfaces' - Build target es2015 for backward compat with older webpack pipelines - Link to local observable-store via file: protocol for monorepo dev Note: Angular 15's webpack still can't handle cross-package class extends in CJS bundles (ReduxDevToolsExtension extends ObservableStore). This is resolved by upgrading samples to Angular 21 which uses esbuild natively. * v3.0.0 — Breaking changes BREAKING CHANGES: - Removed deprecated 'includeStateChangesOnSubscribe' option. Use stateWithPropertyChanges or globalStateWithPropertyChanges instead. - Build target upgraded to ES2022 (native classes, class fields). Consumers must use modern bundlers (Vite, esbuild, webpack 5+). - Minimum Node.js 18+. NEW: - Added destroy() method to ObservableStore. Call from ngOnDestroy() or equivalent lifecycle hook to unregister the service from the global store and complete its state dispatchers. Fixes memory leak where services accumulated in the global services array forever. - Exported stateFunc type from public API. Version bumps: - @codewithdan/observable-store: 2.2.15 → 3.0.0 - @codewithdan/observable-store-extensions: 2.2.9 → 3.0.0 Tests: 62 passed (62 total) * Upgrade React, Vue, and JS samples to latest versions * Upgrade Angular samples to Angular 21 - Upgraded all 4 Angular sample apps from Angular 14-15 to Angular 21 - Converted all components from NgModules to standalone components - Replaced *ngIf/*ngFor with @if/@for control flow syntax - Switched to bootstrapApplication with ApplicationConfig pattern - Using provideRouter/provideHttpClient instead of module imports - Updated build system to @angular/build:application - Moved static assets to public/ directory (Angular 21 convention) - Removed polyfills.ts, test.ts (no longer needed) - Removed @codewithdan/observable-store-extensions Redux DevTools setup - Kept all Observable Store usage patterns identical - All 4 samples build cleanly with ng build Samples upgraded: - angular-simple-store - angular-stateChanged - angular-store - angular-store-edits * Update year * Re-enable Redux DevTools extension in angular-store sample * Use file: references for local modules (v3 not yet published to npm) * Add root build script for local dev setup (modules must be built before samples) * Pin TypeScript to 5.8.x to match API Extractor (removes build warning) * Add zone.js + provideZoneChangeDetection to all Angular samples (fixes empty rendering) * Align Angular samples with Angular 21 scaffold conventions * Add comprehensive test coverage: 103 tests (was 62) New test areas: - destroy(): stateWithPropertyChanges completion, double destroy, multi-service - dispatchState: suppress dispatch, suppress global dispatch - logStateAction: custom actions, history tracking, deep clone verification - resetStateHistory: clear and re-accumulate - logStateChanges: console.log spy verification - Extensions: init() called, allStoreServices tracking - Multiple services: cross-service globalStateChanged, stateSliceSelector isolation - getStateProperty: non-existent property, no-clone mode - getStateSliceProperty: with/without selector, non-existent property - setState error handling: string, number, boolean inputs - State history correctness: beginState/endState, deep clone isolation - ClonerService: primitives, Date, RegExp, getter props, typed arrays, NaN in arrays, nested Dates/RegExp, Map/Set with complex values, complex objects without clone(), empty Map/Set, primitive arrays, deep nesting mutation isolation * Add npm test script to root package.json * Pre-publish prep: CHANGELOG.md, README cleanup, CI update - Added CHANGELOG.md with full v3.0.0 release notes and historical versions - README: removed deprecated includeStateChangesOnSubscribe from settings/global settings - README: added destroy() to API table - README: removed Angular 9/Ivy note (irrelevant for v3/Angular 21) - README: updated React DevTools example (removed legacy router pattern) - README: replaced inline changelog with link to CHANGELOG.md - CI: updated to Node 18/20/22, actions v4, root-level build+test * Add compatibility table to README (Angular 17+, v2.x for older) * Clean up README for v3 - React examples: class components → hooks, CRA → Vite - Removed empty Vue.js placeholder section - Angular: @Injectable() → @Injectable({ providedIn: 'root' }) - Angular production example: environments/environment → isDevMode() - React production example: process.env → import.meta.env.DEV, removed reactRouterHistory - Removed SubSink recommendation (outdated) - Removed isProduction from global settings (never implemented) - Fixed Chrome Web Store URLs (old domain → chromewebstore.google.com) - Fixed 'Obervable' typo in image alt text - Updated DevTools sample reference to angular-store - Updated 'Building the Project' with actual commands * Simplify compatibility section * Pin Angular 14-16 compat to v2.2.15 (last 2.x release) * Fix API docs accuracy and typos - setState: signature now shows Partial<T> | stateFunc<T>, action is optional - dispatchState: return type void (not T) - clearState: added missing dispatchState parameter - stateHistory: type is StateHistory<T>[] (not StateHistory) - Fixed typos: particlar→particular, the the→the, occuring→occurring, isn't change→isn't changed - setState description: deepCloneReturnedState→deepCloneState (matches actual param name) * Add Vue.js and JavaScript sections to README - Add Vue.js usage section with Composition API example - Add JavaScript (no framework) usage section - Update sample applications list to include all 4 frameworks - All examples follow the same pattern: Vite setup, store class, subscribe/cleanup * Remove vue-store sample (empty scaffold, never used Observable Store) * Improve CI: cache both lockfiles, validate samples, add npm publish on tags - Cache both observable-store and extensions lockfiles - Add build-samples job: angular-store, react-store, javascript-demo - Add publish job triggered on version tags (v*) with npm provenance - Publish requires NPM_TOKEN secret in repo settings * refactor: code quality improvements across Observable-Store codebase Core library (observable-store): - Add proper TypeScript types throughout, replacing 'any' with specific types where possible (Record<string, unknown>, generics, union types) - Mark class properties as readonly where appropriate (_settings, _clonerService, _extensions, _stateDispatcher$, _stateWithChangesDispatcher$, stateChanged, etc.) - Add explicit return type annotations to all methods - Use null union types instead of bare null assignments - Improve StateSliceSelector interface with generic type parameter - Use semicolons consistently in interface definitions (StateHistory, StateWithPropertyChanges) - Add JSDoc to ObservableStoreBase singleton class ClonerService (utilities/cloner.service.ts): - Add generic type parameter to deepClone<T> for proper type inference - Replace all 'any' parameter types with proper types (object, unknown, Record<>) - Rename newRegExp() to cloneRegExp() for clarity - Type fixPropertyValue and fixTypes with proper parameter types - Remove redundant type comments, improve code documentation - Fix array type cast in fixTypes to satisfy strict type checking Extensions (observable-store-extensions): - Remove unused Observable/Subscription imports from interfaces.ts - Replace 'any' types in interfaces with 'unknown' where appropriate - Add proper function signatures to ReduxDevtoolsExtensionConfig - Export all interfaces from index.ts (previously only ReduxDevToolsExtension was exported) - Use optional chaining (?.) throughout redux-devtools.extension.ts - Convert Actions enum to const enum for better tree-shaking - Add null types to nullable properties (devToolsExtensionConnection, angularExtension, sub) - Improve angular-devtools-extension.ts with optional chaining and explicit null init - Type the runInZone callback parameter as () => void instead of any All changes maintain full public API stability. 103/103 tests pass. * CI: trigger publish on 'release:' commit message instead of tags * CI: add manual workflow dispatch with dry-run publish option
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TypeScript 4.8+ strict mode throws when
ClonerServiceattempts to clone objects like Dayjs or Moment. These objects havetoJSON()methods that return ISO strings, causingJSON.parse(JSON.stringify())to produce primitives. The subsequentfixTypes()call then attempts property assignment on strings, triggering:Cannot create property '$d' on string.Changes
Detection logic via
isCloneable():toJSON()returning primitives → structural sharingUpdated cloning flow:
deepClone(): checks cloneability before JSON serializationfixPropertyValue(): preserves complex object references, guards against primitive assignmentExample:
This enables Angular 14→15+ migrations for projects using date libraries with custom serialization.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.