fix: preserve subclasses in duplicate() (#2053)#2098
Open
rit3sh-x wants to merge 6 commits into
Open
Conversation
Fix applyMixin copying EventEmitter.prototype.constructor onto the derived prototype, which caused instance.constructor to resolve to EventEmitter instead of Redis/Cluster. With applyMixin corrected to skip the constructor property, Redis and Cluster duplicate() methods can safely use so subclass instances are preserved across duplicate() calls. This enables legitimate subclassing patterns (instrumentation, OpenTelemetry, custom error handling) to survive through duplicate() without monkeypatching. Closes redis#2053
Contributor
|
Thanks for the PR. I will review it and follow up once I have gone through it. |
Contributor
Author
|
@PavelPashov any updates? |
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.
Closes #2053.
Summary
Redis#duplicate()andCluster#duplicate()now preserve the actual class of the instance, so subclasses are returned fromduplicate()instead of plainRedis/Cluster.Root cause
The
applyMixinutility inlib/utils/applyMixin.tscopied every own property from the mixin prototype onto the derived class prototype — including the built-inconstructorproperty. WhenapplyMixin(Redis, EventEmitter)runs at module load,EventEmitter.prototype.constructoroverwritesRedis.prototype.constructor, so any Redis instance hadr.constructor === EventEmitterinstead ofRedis.That is why a naive
new this.constructor(...)fix forduplicate()previously broke Cluster (as noted in Pavel's comment on #2053) — the constructor was wrong at the root.Fix
lib/utils/applyMixin.ts— skip theconstructorkey when copying descriptors, using the canonicalObject.getOwnPropertyDescriptors+delete+Object.definePropertiespattern. Preserves getters/setters, non-enumerable props, and symbol keys.lib/Redis.ts—duplicate()now usesthis.constructorand returnsthis.lib/cluster/index.ts— same treatment forCluster.duplicate(), typed withClusterNode[]/ClusterOptionsfor proper type safety.Example
Test plan
New unit test file
test/unit/duplicate.tswith 15 tests covering:Redis.prototype.constructorcorrectly resolves toRedis(applyMixin fix)Cluster.prototype.constructorcorrectly resolves toClusterredis.duplicate()returns aRedisinstance on plain Redisredis.duplicate()returns a subclass instance when called on a subclassduplicate()ClustersubclassesapplyMixinstill mixes in EventEmitter methods (on,emit,removeAllListeners) onto both Redis and ClusterVerification
npm run build— PASSnpm run test:tsd— PASSRelation to #2073
Independent but complementary. #2073 proposes a
redisClassoption on Cluster to thread a custom class through all internal connections. This PR fixes the underlying inheritance bug that preventednew this.constructor(...)from working in the first place.Either PR can land first — there are no file-level conflicts (different line ranges in
cluster/index.ts).Note
Low Risk
Targeted inheritance/mixin fix with broad unit tests; duplicate() is used internally (e.g. MONITOR, slot refresh) but behavior aligns with documented intent.
Overview
Fixes #2053 so
duplicate()on customRedis/Clustersubclasses returns the same class (instrumentation, extra methods, constructor fields) instead of a plain base instance.Root cause:
applyMixincopiedEventEmitter.prototype.constructorontoRedis/Cluster, soinstance.constructorwas wrong andduplicate()could not safely usenew this.constructor(...).Changes:
applyMixinnow skips copyingconstructorwhen merging prototypes.Redis#duplicate()andCluster#duplicate()instantiate viathis.constructorand are typed to returnthis. New unit tests cover constructors, nested subclasses, overrides, and that EventEmitter APIs still work.Reviewed by Cursor Bugbot for commit 994ec8c. Bugbot is set up for automated code reviews on this repo. Configure here.