-
-
Notifications
You must be signed in to change notification settings - Fork 97
enhance: delegate.getEntities() returns restricted EntitiesInterface #3501
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
Conversation
🦋 Changeset detectedLatest commit: 905ec19 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Change: +90 B (+0.12%) Total Size: 77.7 kB
ℹ️ View Unchanged
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3501 +/- ##
=======================================
Coverage 98.75% 98.76%
=======================================
Files 130 130
Lines 2255 2265 +10
Branches 456 457 +1
=======================================
+ Hits 2227 2237 +10
Misses 13 13
Partials 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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: 905ec19 | Previous: ac05f9d | Ratio |
---|---|---|---|
normalizeLong |
523 ops/sec (±1.68% ) |
509 ops/sec (±1.69% ) |
0.97 |
denormalizeLong |
291 ops/sec (±2.62% ) |
272 ops/sec (±2.85% ) |
0.93 |
denormalizeLong donotcache |
1048 ops/sec (±1.37% ) |
1049 ops/sec (±1.51% ) |
1.00 |
denormalizeShort donotcache 500x |
1607 ops/sec (±0.29% ) |
1592 ops/sec (±0.38% ) |
0.99 |
denormalizeShort 500x |
869 ops/sec (±1.78% ) |
818 ops/sec (±2.32% ) |
0.94 |
denormalizeShort 500x withCache |
6112 ops/sec (±0.31% ) |
6006 ops/sec (±0.16% ) |
0.98 |
queryShort 500x withCache |
2803 ops/sec (±0.19% ) |
2767 ops/sec (±0.54% ) |
0.99 |
infer All |
53057 ops/sec (±0.40% ) |
9936 ops/sec (±0.57% ) |
0.19 |
query All withCache |
6898 ops/sec (±0.31% ) |
7584 ops/sec (±0.42% ) |
1.10 |
denormalizeLong with mixin Entity |
368 ops/sec (±0.39% ) |
270 ops/sec (±2.12% ) |
0.73 |
denormalizeLong withCache |
7747 ops/sec (±0.43% ) |
8181 ops/sec (±0.64% ) |
1.06 |
denormalizeLong All withCache |
6985 ops/sec (±0.26% ) |
7652 ops/sec (±0.26% ) |
1.10 |
denormalizeLong Query-sorted withCache |
6936 ops/sec (±0.16% ) |
7648 ops/sec (±0.30% ) |
1.10 |
denormalizeLongAndShort withEntityCacheOnly |
1835 ops/sec (±0.30% ) |
1787 ops/sec (±0.34% ) |
0.97 |
getResponse |
5794 ops/sec (±1.34% ) |
5582 ops/sec (±1.58% ) |
0.96 |
getResponse (null) |
7072872 ops/sec (±0.78% ) |
7051115 ops/sec (±0.70% ) |
1.00 |
getResponse (clear cache) |
355 ops/sec (±0.60% ) |
264 ops/sec (±2.37% ) |
0.74 |
getSmallResponse |
3104 ops/sec (±0.48% ) |
3030 ops/sec (±0.15% ) |
0.98 |
getSmallInferredResponse |
2344 ops/sec (±0.23% ) |
2315 ops/sec (±0.40% ) |
0.99 |
getResponse Collection |
5666 ops/sec (±0.88% ) |
5671 ops/sec (±1.59% ) |
1.00 |
get Collection |
5478 ops/sec (±0.34% ) |
5777 ops/sec (±0.51% ) |
1.05 |
get Query-sorted |
5772 ops/sec (±0.26% ) |
6587 ops/sec (±0.39% ) |
1.14 |
setLong |
533 ops/sec (±0.50% ) |
549 ops/sec (±0.28% ) |
1.03 |
setLongWithMerge |
244 ops/sec (±0.28% ) |
244 ops/sec (±0.31% ) |
1 |
setLongWithSimpleMerge |
260 ops/sec (±0.27% ) |
258 ops/sec (±0.51% ) |
0.99 |
setSmallResponse 500x |
951 ops/sec (±0.18% ) |
953 ops/sec (±0.35% ) |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
5df5376
to
2e089cd
Compare
8422781
to
c47957c
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 restricts the exposed entities API by replacing the raw entity object with a restricted interface (EntitiesInterface) containing keys() and entries() iterator methods. Key changes include:
- Updating interface definitions and delegate methods to return EntitiesInterface.
- Adjusting implementation in both mutable and immutable delegates.
- Revising tests and documentation to reflect the new API.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/normalizr/src/memo/MemoCache.ts | Removed obsolete inline getDependency function |
packages/normalizr/src/interface.ts | Replaced GetEntities with the new EntitiesInterface |
packages/normalizr/src/delegate/Delegate.ts | Updated getEntities implementation to return EntitiesInterface |
packages/normalizr/src/delegate/Delegate.imm.ts | Adjusted getEntities to support immutable structures |
packages/normalizr/src/delegate/BaseDelegate.ts | Modified abstract getEntities signature and dependency mapping |
packages/endpoint/src/schemas/tests/Object.test.js | Removed unused helper function and unused Map import |
packages/endpoint/src/schemas/tests/All.test.ts | Adjusted test setup for using string keys and immutable updates |
packages/endpoint/src/schemas/Collection.ts | Updated iteration to use EntitiesInterface.keys() instead of Object.keys() |
packages/endpoint/src/schemas/All.ts | Modified queryKey and flatMap logic to work with the new API |
packages/endpoint/src/interface.ts | Updated IQueryDelegate and INormalizeDelegate to use EntitiesInterface |
.changeset/proud-taxes-bake.md | Updated changelog and usage examples for the new getEntities API |
Comments suppressed due to low confidence (1)
packages/normalizr/src/delegate/BaseDelegate.ts:60
- [nitpick] Dynamic method selection using array indexing based on path.length is non-intuitive and error-prone; consider a more explicit mapping or safeguard for unexpected path lengths.
delegate[['', 'getEntitiesObject', 'getEntity', 'getIndex'][path.length]](...path)
if (entities === undefined) return undefined; | ||
return { | ||
keys(): IterableIterator<string> { | ||
return Object.keys(entities) as any; |
Copilot
AI
Jun 7, 2025
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.
[nitpick] Avoid using 'as any' casts; consider explicitly typing the iterator as IterableIterator to improve clarity and maintainability.
return Object.keys(entities) as any; | |
return Object.keys(entities).values(); |
Copilot uses AI. Check for mistakes.
Follow-up to #3468
Motivation
Exposing the full entities data structure is potentially unsafe, and has unbounced interface implications for its implementation.
Restrict exposed surface area to looping behavior.
Solution
Return value is a restricted interface with
keys()
andentries()
iterator methods.Update both
IQueryDelegate
, andINormalizeDelegate
as we try to match their interfaces as much as possible.Before
After
500% performance improvement in schema.All().buildQueryKey