-
-
Notifications
You must be signed in to change notification settings - Fork 97
feat: Normalize into immutablejs state #3506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Size Change: +6 B (+0.01%) Total Size: 77.7 kB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Benchmark
Benchmark suite | Current: c90abbc | Previous: b30806d | Ratio |
---|---|---|---|
normalizeLong |
520 ops/sec (±1.38% ) |
524 ops/sec (±1.32% ) |
1.01 |
denormalizeLong |
285 ops/sec (±2.56% ) |
284 ops/sec (±2.16% ) |
1.00 |
denormalizeLong donotcache |
1048 ops/sec (±0.80% ) |
1009 ops/sec (±0.88% ) |
0.96 |
denormalizeShort donotcache 500x |
1561 ops/sec (±1.04% ) |
1596 ops/sec (±0.16% ) |
1.02 |
denormalizeShort 500x |
848 ops/sec (±1.93% ) |
863 ops/sec (±1.78% ) |
1.02 |
denormalizeShort 500x withCache |
6512 ops/sec (±0.24% ) |
6357 ops/sec (±0.76% ) |
0.98 |
queryShort 500x withCache |
2750 ops/sec (±0.32% ) |
2762 ops/sec (±0.32% ) |
1.00 |
buildQueryKey All |
55121 ops/sec (±0.36% ) |
55270 ops/sec (±0.39% ) |
1.00 |
query All withCache |
6805 ops/sec (±0.94% ) |
6788 ops/sec (±0.46% ) |
1.00 |
denormalizeLong with mixin Entity |
276 ops/sec (±1.89% ) |
281 ops/sec (±1.78% ) |
1.02 |
denormalizeLong withCache |
6658 ops/sec (±0.33% ) |
6522 ops/sec (±0.16% ) |
0.98 |
denormalizeLong All withCache |
6610 ops/sec (±0.43% ) |
6416 ops/sec (±0.16% ) |
0.97 |
denormalizeLong Query-sorted withCache |
6885 ops/sec (±0.20% ) |
6861 ops/sec (±0.39% ) |
1.00 |
denormalizeLongAndShort withEntityCacheOnly |
1710 ops/sec (±0.30% ) |
1702 ops/sec (±0.21% ) |
1.00 |
getResponse |
5859 ops/sec (±1.54% ) |
5587 ops/sec (±1.36% ) |
0.95 |
getResponse (null) |
6899882 ops/sec (±1.63% ) |
7377465 ops/sec (±0.61% ) |
1.07 |
getResponse (clear cache) |
283 ops/sec (±2.19% ) |
276 ops/sec (±2.00% ) |
0.98 |
getSmallResponse |
3086 ops/sec (±0.71% ) |
3045 ops/sec (±0.57% ) |
0.99 |
getSmallInferredResponse |
2340 ops/sec (±0.28% ) |
2379 ops/sec (±0.43% ) |
1.02 |
getResponse Collection |
5715 ops/sec (±1.52% ) |
5646 ops/sec (±1.30% ) |
0.99 |
get Collection |
5846 ops/sec (±0.34% ) |
5936 ops/sec (±0.35% ) |
1.02 |
get Query-sorted |
6838 ops/sec (±0.17% ) |
6636 ops/sec (±0.34% ) |
0.97 |
setLong |
533 ops/sec (±0.32% ) |
526 ops/sec (±0.60% ) |
0.99 |
setLongWithMerge |
245 ops/sec (±0.14% ) |
241 ops/sec (±0.39% ) |
0.98 |
setLongWithSimpleMerge |
262 ops/sec (±0.42% ) |
262 ops/sec (±0.38% ) |
1 |
setSmallResponse 500x |
940 ops/sec (±0.35% ) |
931 ops/sec (±0.46% ) |
0.99 |
This comment was automatically generated by workflow using github-action-benchmark.
24e119b
to
008fc9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the normalization process to use a more streamlined approach where NormalizeDelegate
directly implements the NormalizedSchema
interface, eliminating the need for a separate return object. The key change moves from creating a separate normalized result object to having the delegate itself serve as the normalized schema.
Key changes:
- Refactored
normalize()
function to return the delegate directly instead of a separate result object - Updated
NormalizeDelegate
to implementNormalizedSchema
interface with declared properties - Reorganized internal tracking from separate maps to a nested structure under
this.new
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
packages/normalizr/src/normalize/normalize.ts | Simplified normalize function to return delegate directly instead of creating separate result object |
packages/normalizr/src/normalize/NormalizeDelegate.ts | Enhanced delegate to implement NormalizedSchema interface and reorganized internal state tracking |
); | ||
const visit = getVisit(delegate); | ||
delegate.result = visit(schema, input, input, undefined, args); | ||
return delegate as any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using 'as any' type assertion bypasses TypeScript's type checking. Consider defining proper type annotations or ensuring the delegate properly implements the expected return type to maintain type safety.
return delegate as any; | |
return { | |
result: delegate.result, | |
entities: delegate.entities, | |
indexes: delegate.indexes, | |
entitiesMeta: delegate.entitiesMeta, | |
}; |
Copilot uses AI. Check for mistakes.
// entities: { ...state.entities }, | ||
// indexes: { ...state.indexes }, | ||
// entitiesMeta: { ...state.entitiesMeta }, | ||
// }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This large block of commented code should be removed as it adds clutter and doesn't provide value. If this is temporary implementation notes, consider using TODO comments instead.
// }; | |
// TODO: Assign to this.normalized if/when needed. |
Copilot uses AI. Check for mistakes.
{ | ||
// declare readonly normalized: NormalizedSchema<E, R>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commented declare statement should be removed as it's unused and adds confusion about the class structure.
// declare readonly normalized: NormalizedSchema<E, R>; |
Copilot uses AI. Check for mistakes.
Motivation
Solution
Open questions