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.
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
keyChange update key/connectionID for faster lookup #520
keyChange update key/connectionID for faster lookup #520
Changes from 4 commits
192ac17
7207211
2181472
f69045f
fba0331
f5137ca
3fb6899
fe17400
238cced
5af3812
7fec278
d55dcd1
d4569dd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check failure on line 323 in lib/OnyxUtils.ts
GitHub Actions / lint
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.
The JSDoc still needs to be updated.
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.
@chrispader what needs to be updated? can you help me I don't recall
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.
Just the name and maybe we can change the description to
It extracts the non-numeric collection identifier of a given key.
or sth.getCollectionKey
instead ofgetPureKey
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.
I find the name of this function a bit misleading. What is a pure key? Why is the "collection part" pure?
Shouldn't we call this sth like:
getCollectionKeyFromKey
?Also, think we should not just remove the non-numeric part to get the key part that's defining the collection.
Technically, it would be possible to have a collection called something like
101products_
orreport5_
Instead we should scrap the "collection part" based on the valid/allowed collection keys configured here:
react-native-onyx/lib/OnyxUtils.ts
Lines 109 to 113 in d7ab226
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.
I like those suggestions, that would be more futureproof
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.
@chrispader I can make that change, that name I just keep it when I was doing some proof of concepts, I can change it.
I also just change it, to add more support to cases like "report_add", "report_test1". That function is to clean, and get the main key, from "report_123" get "report_", using the key from keys.COLLECTION I would have to loop through the collection keys, and check the startsWith.
This comment was marked as outdated.
Sorry, something went wrong.
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.
also NAB and just code style discussion / how explicit we want to be.
It might be more explicit:
Here we explicitly check for a collection key, instead of implicitly doing that by having a fallback check, which is more verbose
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.
Most of the time the key exists and isn't a collection, also when it enters the main loop
it also checks for
isCollectionKey
, wanna avoid extra computationThere 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.
okay, makes sense. If its also that way for performance reasons, might be worth to add a comment to avoid future regressions
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.
Something that I'm just realize, is when sending a key that is
report_1234
it sends to any component listening to that specific key, but it also sends updates to all components listening toreport_
, I'm looking into a way to update my code + keep the performanceThis comment was marked as outdated.
Sorry, something went wrong.