keyChange update key/connectionID for faster lookup #520

gedu · 2024-03-26T11:42:01Z

Details

Following proposal: Expensify/App#35809

Related Issues

Manual Tests

Expensify/App#36226 (comment)

Author Checklist

I linked the correct issue in the ### Related Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

github-actions · 2024-03-26T11:42:12Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

tgolen · 2024-03-28T14:36:25Z

I'm placing this on HOLD right now in order for QA to be done on the latest version of Onyx without more un-QAed PRs from being merged.

gedu · 2024-04-02T05:25:25Z

I have read the CLA Document and I hereby sign the CLA

Julesssss · 2024-04-02T11:41:50Z

lib/OnyxUtils.d.ts

+ */
+declare function storeKeyByConnections(connectionID: string, key: OnyxKey): void;
+
+declare function deleteKeyByConnections(connectionID: number): void;


NAB: Just incase these get refactored or a new function is created which separates them, would you mind adding a function comment for deleteKeyByConnections too please?

lib/OnyxUtils.js

Julesssss · 2024-04-02T11:46:39Z

gedu requested a review from https://github.com/orgs/Expensify/teams/pullerbear as a code owner 6 hours ago

Is this off HOLD?

cc @mountiny who is assigned to the linked issue

mountiny · 2024-04-02T20:31:18Z

It is not off hold until we merge the onyx bump and deploy it, but we can review in the meantime

Julesssss · 2024-04-29T13:38:20Z

It is not off hold until we merge the onyx bump and deploy it, but we can review in the meantime

I think we've likely bumped Onyx by now?

# Conflicts: # lib/OnyxUtils.d.ts # lib/OnyxUtils.ts

gedu · 2024-04-30T09:30:54Z

Conflicts fixed

gedu · 2024-05-09T10:06:58Z

@mountiny can we keep reviewing this PR?
Will be more improvements: https://callstack-hq.slack.com/archives/C05LX9D6E07/p1715178705054759

lib/Onyx.ts

lib/OnyxUtils.ts

lib/Onyx.ts

hannojg

LGTM (just some stylistic discussion, but the main improvement is 🔥!)

lib/OnyxUtils.ts

hannojg · 2024-05-10T10:00:19Z

lib/OnyxUtils.ts

+    let stateMappingKeys = onyxKeyToConnectionIDs.get(key);
+
+    if (!stateMappingKeys) {
+        // Getting the collection key from the specific key because only collection keys were stored in the mapping.
+        stateMappingKeys = onyxKeyToConnectionIDs.get(getPureKey(key));
+        if (!stateMappingKeys) {
+            return;
+        }
+    }
+


also NAB and just code style discussion / how explicit we want to be.

It might be more explicit:

const connectionKey = isCollectionKey(key) ? getPureKey(key) : key; const stateMappingKeys = onyxKeyToConnectionIDs.get(connectionKey) if (!stateMappingKeys) { return; }

Here we explicitly check for a collection key, instead of implicitly doing that by having a fallback check, which is more verbose

Most of the time the key exists and isn't a collection, also when it enters the main loop

for (let i = 0; i < stateMappingKeys.length; i++) {

it also checks for isCollectionKey, wanna avoid extra computation

okay, makes sense. If its also that way for performance reasons, might be worth to add a comment to avoid future regressions

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 to report_, I'm looking into a way to update my code + keep the performance

mountiny · 2024-05-20T19:56:33Z

We are handling an onyx update in this PR Expensify/App#42057 Lets wait for that to be in staging before merging this change

chrispader

LGTM aside form one required JSDoc comment update

lib/OnyxUtils.ts

+ * @param {OnyxKey} key - The key to process.
+ * @return {string} The pure key without any numeric
+ */
+function getPureKey(key: OnyxKey): string {


lib/OnyxUtils.ts

+    let stateMappingKeys = onyxKeyToConnectionIDs.get(key);
+
+    if (!stateMappingKeys) {
+        // Getting the collection key from the specific key because only collection keys were stored in the mapping.
+        stateMappingKeys = onyxKeyToConnectionIDs.get(getPureKey(key));
+        if (!stateMappingKeys) {
+            return;
+        }
+    }
+


chrispader · 2024-06-15T12:59:03Z

lib/OnyxUtils.ts

+/**
+ * It extracts the pure, non-numeric part of a given key.
+ *
+ * For example:
+ * - `getPureKey("report_123")` would return "report_"
+ * - `getPureKey("report")` would return "report"
+ * - `getPureKey("report_")` would return "report_"
+ * - `getPureKey(null)` would return ""
+ *
+ * @param {OnyxKey} key - The key to process.
+ * @return {string} The pure key without any numeric
+ */


The JSDoc still needs to be updated.

@chrispader what needs to be updated? can you help me I don't recall

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 of getPureKey

gedu · 2024-06-17T13:28:11Z

Cool, will update, fix the comment and check if this makes sense

# Conflicts: # lib/OnyxUtils.ts

gedu · 2024-06-20T09:05:54Z

All good from my side, this is still valid, just need to double check @chrispader comment about JSDoc

mountiny · 2024-06-20T12:26:10Z

Bumped for a review from @chrispader

chrispader

LGTM! Still only the requested JSDoc comment update

gedu · 2024-06-21T10:48:06Z

@chrispader updated JSDoc

chrispader

Thanks for the changes. LGTM!

lib/OnyxUtils.ts

mountiny

Thanks for adding the tests.

Julesssss · 2024-06-28T13:55:00Z

I think we're ready to go here, are we waiting on any final reviews?

gedu · 2024-07-02T09:00:33Z

Any news? @mountiny @Julesssss

Julesssss · 2024-07-02T10:38:43Z

Any news? @mountiny @Julesssss

Sorry, I missed this one. Can you please create an onyx bump PR and assign me please?

Julesssss · 2024-07-02T10:40:42Z

Ah shit. Your first two commits weren't signed. Could you please raise another PR against this issue? Please tag us again for another review.

gedu · 2024-07-02T11:21:49Z

Ohhh maan, ok on my way, sorry for that

gedu · 2024-07-02T12:49:08Z

@Julesssss new PR created: #565

Storing key/connectionID for faster lookup

192ac17

gedu mentioned this pull request Mar 26, 2024

[HOLD] Patch for getting connection IDs by key Expensify/App#36226

Closed

50 tasks

tgolen changed the title ~~keyChange update key/connectionID for faster lookup~~ [HOLD] keyChange update key/connectionID for faster lookup Mar 28, 2024

gedu marked this pull request as ready for review April 2, 2024 05:25

gedu requested a review from a team as a code owner April 2, 2024 05:25

melvin-bot bot requested review from Julesssss and removed request for a team April 2, 2024 05:26

Julesssss reviewed Apr 2, 2024

View reviewed changes

lib/OnyxUtils.js Outdated Show resolved Hide resolved

mountiny self-requested a review April 2, 2024 20:30

mountiny assigned gedu Apr 2, 2024

Documented new functions

7207211

Merge branch 'main' into edu/keychange_improvement

2181472

# Conflicts: # lib/OnyxUtils.d.ts # lib/OnyxUtils.ts

mountiny changed the title ~~[HOLD] keyChange update key/connectionID for faster lookup~~ keyChange update key/connectionID for faster lookup May 9, 2024

mountiny reviewed May 9, 2024

View reviewed changes

lib/Onyx.ts Show resolved Hide resolved

lib/OnyxUtils.ts Outdated Show resolved Hide resolved

lib/OnyxUtils.ts Outdated Show resolved Hide resolved

lib/OnyxUtils.ts Outdated Show resolved Hide resolved

lib/OnyxUtils.ts Outdated Show resolved Hide resolved

mountiny requested a review from mollfpr May 9, 2024 13:06

Updated JDocs

f69045f

mountiny reviewed May 9, 2024

View reviewed changes

lib/Onyx.ts Outdated Show resolved Hide resolved

gedu added 2 commits May 9, 2024 20:32

add clarification to a comment

fba0331

Merge branch 'main' into edu/keychange_improvement

f5137ca

hannojg reviewed May 10, 2024

View reviewed changes

Make it more readable and performant not using _.without

3fb6899

mountiny changed the title ~~keyChange update key/connectionID for faster lookup~~ [HOLD for App onyx bump] keyChange update key/connectionID for faster lookup May 29, 2024

chrispader suggested changes Jun 15, 2024

View reviewed changes

Merge branch 'main' into edu/keychange_improvement

7fec278

# Conflicts: # lib/OnyxUtils.ts

mountiny changed the title ~~[HOLD for App onyx bump] keyChange update key/connectionID for faster lookup~~ keyChange update key/connectionID for faster lookup Jun 20, 2024

chrispader approved these changes Jun 20, 2024

View reviewed changes

Updated JSDoc

d55dcd1

chrispader approved these changes Jun 21, 2024

View reviewed changes

Julesssss previously approved these changes Jun 24, 2024

View reviewed changes

mountiny requested changes Jun 25, 2024

View reviewed changes

lib/OnyxUtils.ts Outdated Show resolved Hide resolved

Updated collection keys to handle multiple underscores

d4569dd

gedu dismissed Julesssss’s stale review via d4569dd June 26, 2024 10:21

mountiny approved these changes Jun 26, 2024

View reviewed changes

chrispader approved these changes Jun 26, 2024

View reviewed changes

Julesssss approved these changes Jun 28, 2024

View reviewed changes

Julesssss approved these changes Jul 2, 2024

View reviewed changes

Julesssss mentioned this pull request Jul 2, 2024

[HOLD for payment 2024-07-17] LOW: [HOLD] Create secondary maps for connection keys in Onyx Expensify/App#35809

Closed

gedu mentioned this pull request Jul 2, 2024

key changed improvement #565

Merged

42 tasks

Julesssss closed this Jul 2, 2024

Julesssss mentioned this pull request Jul 4, 2024

[NoQA] chore: bump onyx to 2.0.55 Expensify/App#44847

Merged

50 tasks

keyChange update key/connectionID for faster lookup #520

keyChange update key/connectionID for faster lookup #520

Conversation

gedu commented Mar 26, 2024

Details

Related Issues

Manual Tests

Author Checklist

Screenshots/Videos

github-actions bot commented Mar 26, 2024 • edited Loading

tgolen commented Mar 28, 2024

gedu commented Apr 2, 2024

Choose a reason for hiding this comment

Julesssss commented Apr 2, 2024

mountiny commented Apr 2, 2024

Julesssss commented Apr 29, 2024

gedu commented Apr 30, 2024

gedu commented May 9, 2024

hannojg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

mountiny commented May 20, 2024

chrispader left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gedu commented Jun 17, 2024

gedu commented Jun 20, 2024

mountiny commented Jun 20, 2024

chrispader left a comment

Choose a reason for hiding this comment

gedu commented Jun 21, 2024

chrispader left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

Julesssss commented Jun 28, 2024

gedu commented Jul 2, 2024

Julesssss commented Jul 2, 2024

Julesssss commented Jul 2, 2024 • edited Loading

gedu commented Jul 2, 2024

gedu commented Jul 2, 2024

github-actions bot commented Mar 26, 2024 •

edited

Loading

Julesssss commented Jul 2, 2024 •

edited

Loading