-
Notifications
You must be signed in to change notification settings - Fork 71
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
Fix getCollectionKey()
and splitCollectionMemberKey()
logic to consider more possibilities of keys / collection keys
#581
Fix getCollectionKey()
and splitCollectionMemberKey()
logic to consider more possibilities of keys / collection keys
#581
Conversation
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.
Thanks @fabioh8010 and @VickyStash for a quick fix. I understand the concern about the loop, but I am not sure if there is a better way to handle this either. This way we at least can exit the logic as soon as we find the collection key so I would assume the performance should not be really degraded
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 looks fine to me, I also don’t see a better way to handle this logic. I am not too concerned about the perf of this since it will only be slightly slower for the edge cases that it now handles. Also keys are not very long and it will only cause a few iterations with a substring call.
@s77rt please test this in App using steps from Expensify/App#48777. Even if this is still marked as draft. Thank you! |
lib/OnyxUtils.ts
Outdated
const collectionKey = getCollectionKey(key); | ||
|
||
if (underscoreIndex === -1) { | ||
throw new Error(`Invalid ${key} key provided, only collection keys are allowed.`); | ||
if (key === collectionKey && !isCollectionKey(key)) { | ||
throw new Error(`Invalid '${key}' key provided, only collection keys are allowed.`); |
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.
NAB. It's a little confusing that we get a collectionKey
then we check if it's a real collection key !isCollectionKey(key)
. I would assume collectionKey
to be an actual collection key already.
There is also a contradiction, in getCollectionKey
, report
is treated as a valid collection key but in splitCollectionMemberKey
it's not. (that's on main
already)
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.
Makes sense, I've update getCollectionKey()
to throw an error if the key is not a collection one, please have a look!
Tested with Expensify/App#49147 and I confirm it works |
* | ||
* @param {OnyxKey} key - The key to process. | ||
* @return {string} The pure key without any numeric | ||
* @return {string} The plain collection key. |
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.
NAB: Would be good to update the docs here that the function can throw
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.
@mountiny I will address it in a follow-up later (together with another tiny improvement)
🚀Published to npm in v2.0.68 |
Details
This PR:
getCollectionKey()
andsplitCollectionMemberKey()
logic to consider some keys / collection keys edge cases with underscore that weren't being considered before e.g.report__FAKE
andcards_-1_Expensify Card
, thus fixing [HOLD for payment 2024-09-24][$250] InvestigateuseOnyx
crash when changingpolicy_-1
key topolicy__FAKE_
App#48777 and [Workspace Feeds] App crashes after enabling Expensify Card App#49147.canEvict
tests inuseOnyxTest.ts
as they were actually wrong and were only working because of the faulty logic this PR is fixing.Related Issues
Expensify/App#48777
Expensify/App#49147
Automated Tests
Manual Tests
Author Checklist
### Related Issues
section aboveTests
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop