Skip to content

Pass withOnyx instance props to selectors#247

Merged
tgolen merged 6 commits into
mainfrom
tgolen-selector-props
May 17, 2023
Merged

Pass withOnyx instance props to selectors#247
tgolen merged 6 commits into
mainfrom
tgolen-selector-props

Conversation

@tgolen

@tgolen tgolen commented May 5, 2023

Copy link
Copy Markdown
Collaborator

Details

This is needed for threads. See the discussion here

Fixes

$ Expensify/App#18489

Automated Tests

Everything is covered by tests

Linked PRs

@grgia will be implementing this

@tgolen tgolen requested a review from grgia May 5, 2023 13:48
@tgolen tgolen self-assigned this May 5, 2023
@tgolen tgolen requested a review from a team as a code owner May 5, 2023 13:48
@melvin-bot melvin-bot Bot requested review from PauloGasparSv and removed request for a team May 5, 2023 13:49
grgia
grgia previously approved these changes May 5, 2023
Comment thread lib/Onyx.js Outdated
PauloGasparSv
PauloGasparSv previously approved these changes May 5, 2023

@PauloGasparSv PauloGasparSv left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left a single comment but NAB IMO

Comment thread lib/Onyx.js Outdated
Comment thread lib/Onyx.js Outdated
@tgolen tgolen dismissed stale reviews from PauloGasparSv and grgia via cd31f6c May 5, 2023 16:09
PauloGasparSv
PauloGasparSv previously approved these changes May 5, 2023

@PauloGasparSv PauloGasparSv left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tgolen

tgolen commented May 5, 2023

Copy link
Copy Markdown
Collaborator Author

OK, I found with @grgia that we needed to pass the state instead of props, and I also found a spot where I wasn't passing the parameter to the selector properly. I've updated tests and docs

@chiragsalian chiragsalian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

havent tested, LGTM. this will be a nice addition.

@JmillsExpensify

Copy link
Copy Markdown

What's left to merge this one?

@tgolen

tgolen commented May 9, 2023

Copy link
Copy Markdown
Collaborator Author

@grgia has been testing this pretty extensively on a local branch. @grgia Do you think it's working as intended or does this need more changes before it can be merged?

@grgia

grgia commented May 9, 2023

Copy link
Copy Markdown
Contributor

@tgolen I wasn't able to get it working on my branch using the selector / props - HOC is here if you want to take a look - Expensify/App#18638

@grgia

grgia commented May 9, 2023

Copy link
Copy Markdown
Contributor

I'm not sure it's working as expected, I only seem to get {isLoading: true} no matter the key/number of onyx connect/prop combo

@tgolen

tgolen commented May 10, 2023

Copy link
Copy Markdown
Collaborator Author

One way I tested this to ensure it was working was to edit HeaderView.js and:

  1. Remove all keys from withOnyx() config
  2. Added this single key in the withOnyx() config
        reports: {
            key: ONYXKEYS.COLLECTION.REPORT,
            selector: (reports, props) => {
                console.log('PROPS', props);
                return props && props.report && _.findWhere(reports, {reportID: props.report.reportID})
            }
        }
  1. Verified that I saw it was able to find the report and {loading: false} eventually appeared in the console

@tgolen

tgolen commented May 17, 2023

Copy link
Copy Markdown
Collaborator Author

Since this is working with the unit tests and in the real-world, I'm going to go ahead and merge this. Maybe one day I will get the chance to implement it in NewDot

@tgolen tgolen merged commit 0d81de6 into main May 17, 2023
@tgolen tgolen deleted the tgolen-selector-props branch May 23, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants