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

remove fetching all venues across the codebase #1620

Merged
merged 91 commits into from
Jul 21, 2021

Conversation

mike-lvov
Copy link
Contributor

@mike-lvov mike-lvov commented Jun 21, 2021

Replace it with useRelativeVenues instead

partially fixes https://github.com/sparkletown/internal-sparkle-issues/issues/839

@mike-lvov mike-lvov added 💥 tech-debt 🏎️ performance For performance related issues/improvements labels Jun 21, 2021
@mike-lvov mike-lvov requested a review from a team June 21, 2021 22:38
@mike-lvov mike-lvov self-assigned this Jun 21, 2021
@mike-lvov mike-lvov temporarily deployed to feature-preview June 21, 2021 22:38 Inactive
@codeclimate
Copy link

codeclimate bot commented Jun 21, 2021

Code Climate has analyzed commit c7af3b2 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

View more on Code Climate.

@github-actions
Copy link

github-actions bot commented Jun 21, 2021

Visit the preview URL for this PR (updated for commit c7af3b2):

https://co-reality-staging--preview-pr-1620-7nfntm93.web.app

(expires Wed, 28 Jul 2021 19:02:47 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link
Contributor

@0xdevalias 0xdevalias left a comment

Choose a reason for hiding this comment

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

Thoughts/comments/changes/etc.

I'm not sure that any of the usages in the admin components/etc will work currently as this code is, because there is no RelatedVenuesProvider in-scope for them (and to do so requires being able to give it a specific venueId).

I haven't looked too deeply into things.. but I expect that most if not all usages of it across the admin components may be wrong/not a good usage of it. It wasn't really written with the admin panel in mind, and I suspect in most cases it's just going to lead to needlessly overfetching data.

src/components/organisms/AppRouter/AppRouter.jsx Outdated Show resolved Hide resolved
src/components/templates/Playa/Playa.tsx Outdated Show resolved Hide resolved
src/pages/VenuePage/TemplateWrapper.tsx Outdated Show resolved Hide resolved
src/pages/VenuePage/VenuePage.tsx Outdated Show resolved Hide resolved
src/pages/VenuePage/VenuePage.tsx Outdated Show resolved Hide resolved
src/hooks/useRoom.ts Outdated Show resolved Hide resolved
src/hooks/useRoom.ts Show resolved Hide resolved
src/hooks/useRoom.ts Outdated Show resolved Hide resolved
src/pages/Admin/Admin_v2.tsx Outdated Show resolved Hide resolved
src/pages/Admin/Admin_v2.tsx Outdated Show resolved Hide resolved
@goran-peoski-work goran-peoski-work temporarily deployed to feature-preview July 21, 2021 16:56 Inactive
@goran-peoski-work goran-peoski-work temporarily deployed to feature-preview July 21, 2021 17:10 Inactive
Copy link
Contributor Author

@mike-lvov mike-lvov left a comment

Choose a reason for hiding this comment

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

A few last changes and good to ship

src/hooks/useConnectOwnedVenues.tsx Outdated Show resolved Hide resolved
src/hooks/useConnectOwnedVenues.tsx Outdated Show resolved Hide resolved
src/utils/selectors.ts Outdated Show resolved Hide resolved
src/hooks/useConnectOwnedVenues.tsx Outdated Show resolved Hide resolved
src/hooks/useConnectOwnedVenues.tsx Outdated Show resolved Hide resolved
src/hooks/useConnectOwnedVenues.tsx Outdated Show resolved Hide resolved
Co-authored-by: mike-lvov <63194656+mike-lvov@users.noreply.github.com>
Co-authored-by: mike-lvov <63194656+mike-lvov@users.noreply.github.com>
@goran-peoski-work goran-peoski-work temporarily deployed to feature-preview July 21, 2021 18:34 Inactive
Copy link
Contributor Author

@mike-lvov mike-lvov left a comment

Choose a reason for hiding this comment

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

Approved, once reused the selector properly. I can't approve my own PR, but I'll ask @jonboutelle to do it

src/hooks/useConnectOwnedVenues.tsx Outdated Show resolved Hide resolved
src/hooks/useConnectOwnedVenues.tsx Outdated Show resolved Hide resolved
@mike-lvov mike-lvov dismissed stale reviews from denisdimitrov and 0xdevalias July 21, 2021 18:38

Looks like all of the things were done

Co-authored-by: mike-lvov <63194656+mike-lvov@users.noreply.github.com>
@goran-peoski-work goran-peoski-work temporarily deployed to feature-preview July 21, 2021 18:59 Inactive
Copy link

@jonboutelle jonboutelle left a comment

Choose a reason for hiding this comment

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

gogogo

@mike-lvov mike-lvov merged commit 060c85d into staging Jul 21, 2021
@mike-lvov mike-lvov deleted the fix/remove-firebase-venues-fetching branch July 21, 2021 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏎️ performance For performance related issues/improvements 💥 tech-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants