Skip to content
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

Pass withOnyx instance props to selectors #247

Merged
merged 6 commits into from
May 17, 2023
Merged

Conversation

tgolen
Copy link
Collaborator

@tgolen tgolen commented May 5, 2023

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
lib/Onyx.js Outdated Show resolved Hide resolved
PauloGasparSv
PauloGasparSv previously approved these changes May 5, 2023
Copy link

@PauloGasparSv PauloGasparSv left a comment

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

lib/Onyx.js Outdated Show resolved Hide resolved
lib/Onyx.js Outdated Show resolved Hide resolved
@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
Copy link

@PauloGasparSv PauloGasparSv left a comment

Choose a reason for hiding this comment

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

LGTM!

@tgolen
Copy link
Collaborator Author

tgolen commented May 5, 2023

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

Copy link
Contributor

@chiragsalian chiragsalian left a comment

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

What's left to merge this one?

@tgolen
Copy link
Collaborator Author

tgolen commented May 9, 2023

@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
Copy link
Contributor

grgia commented May 9, 2023

@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
Copy link
Contributor

grgia commented May 9, 2023

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
Copy link
Collaborator Author

tgolen commented May 10, 2023

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
Copy link
Collaborator Author

tgolen commented May 17, 2023

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