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

refactor: migrate invite link to new router #996

Merged
merged 9 commits into from
Mar 11, 2024

Conversation

adamstankiewicz
Copy link
Member

@adamstankiewicz adamstankiewicz commented Mar 7, 2024

Description

https://2u-internal.atlassian.net/browse/ENT-8479

  • Re-establishes a route for the universal link (enterprise invite page), but using a route loader function instead. The UI element for the route only renders if there's an error.
  • Removes prior code related to the EnterpriseInvitePage.

For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)
  • Ensure English strings are marked for translation. See documentation for more details.

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

Copy link

codecov bot commented Mar 7, 2024

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 84.26%. Comparing base (02a59f2) to head (5d92bd2).

Files Patch % Lines
src/components/app/routes/createAppRouter.jsx 0.00% 3 Missing ⚠️
Additional details and impacted files
@@                        Coverage Diff                         @@
##           feat/react-query-route-loaders     #996      +/-   ##
==================================================================
- Coverage                           84.30%   84.26%   -0.04%     
==================================================================
  Files                                 364      364              
  Lines                                7825     7813      -12     
  Branches                             1913     1909       -4     
==================================================================
- Hits                                 6597     6584      -13     
- Misses                               1185     1186       +1     
  Partials                               43       43              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adamstankiewicz adamstankiewicz changed the base branch from feat/react-query-route-loaders to ags/ent-8597 March 8, 2024 17:03
Base automatically changed from ags/ent-8597 to feat/react-query-route-loaders March 8, 2024 19:03
@adamstankiewicz adamstankiewicz marked this pull request as ready for review March 8, 2024 21:33
Copy link
Contributor

@pwnage101 pwnage101 left a comment

Choose a reason for hiding this comment

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

I had to ramp up on react router to understand any of this, and got a bit confused until I realized this PR wasn't targeting master, but overall seems legit. Sorry I don't have much else to add, without spending a bunch more time on it.

It seems like the core change here is moving the universal link auth/redirect logic into a special react router callback instead of keeping it embedded it in the component itself? I'm guessing this allows rendering of the component to be completely bypassed in most cases, and there's some sort of performance benefit?

@adamstankiewicz adamstankiewicz merged commit a825648 into feat/react-query-route-loaders Mar 11, 2024
6 of 7 checks passed
@adamstankiewicz adamstankiewicz deleted the ags/ent-8479 branch March 11, 2024 13:40
@adamstankiewicz
Copy link
Member Author

It seems like the core change here is moving the universal link auth/redirect logic into a special react router callback instead of keeping it embedded it in the component itself? I'm guessing this allows rendering of the component to be completely bypassed in most cases, and there's some sort of performance benefit?

@pwnage101 Yeah, at a super high-level, the work of the feature branch is to switch the existing paradigm of "render then fetch" to a "fetch then render" paradigm instead, where data is resolved from APIs before displaying a route, instead of after, effectively eliminating the majority of loading spinners/skeleton states in favor of a YouTube-esque progress bar across the top of the UI.

Previously, we had many request waterfalls where certain groups of API calls weren't made until other groups of API calls further up the component tree were resolved, even when there's no dependent data between the API calls. With the migration to route loaders and React Query, all API calls are made in parallel (for the most part) and stored in the client-side query cache for 20 seconds, eliminating the need to make the same API call(s) for data we already have in many places when navigating between page routes.

In the case of the invite link specifically, you're correct the component associated with the route will not be displayed if the invite link API call was successful; if it errors, the route loader resolves to null instead of throwing a redirect to the dashboard, whereby the associated component to the route is rendered (for invite page, an error screen).

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.

3 participants