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.

Sign up for GitHub

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

Check for loading auth before rendering apollo provider #1945

Closed
wants to merge 1 commit into from
Closed

Check for loading auth before rendering apollo provider #1945

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/web/src/apollo/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,16 @@ const ApolloProviderWithFetchConfig: React.FunctionComponent<{
config?: Omit<ApolloClientOptions<InMemoryCache>, 'cache'>
}> = ({ config = {}, children }) => {
const { uri, headers } = useFetchConfig()
const { getToken, type: authProviderType, isAuthenticated } = useAuth()
const {
getToken,
type: authProviderType,
isAuthenticated,
loading,
} = useAuth()

if (loading) {
return null
Copy link
Member

Choose a reason for hiding this comment

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

I don't want to stop rendering here. I want rendering to continue down to the router so it can show the whileLoading() stuff.

Copy link
Member

Choose a reason for hiding this comment

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

redwood/packages/router/src/router.tsx

Lines 65 to 67 in 42ec5eb

if (loading) {
return whileLoading()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tobbe I tried putting in the same whileLoading logic on the public routes in router.tsx as well and even used a private route in this issue and in both cases I end up stuck on the "/" route even though the address bar shows "/after-auth" as I demonstrated in #1892

So far this was the only thing that fixed the issue.

redwood/packages/web/src/components/RedwoodProvider.tsx

Lines 40 to 45 in 53349fd

// This really sucks because rendering is completely blocked whilst we're
// restoring authentication. In a lot of cases that's OK since the token is stored
// in localstorage or a secure cookie.
if (loading) {
return null
}

Based on this comment from 0.21 someone has always wanted it removed but I've pretty much narrowed it down to the one thing causing both issues in #1892.

Copy link
Contributor Author

@o0charlie0o o0charlie0o Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@Tobbe I think the real issue here is probably since rendering is no longer blocked in the RedwoodProvider during auth loading, Location sets it's context before replaceState is called in restoreAuthState and then doesn't reset it's context since replaceState doesn't trigger the popstate event. I've seen some workarounds for this:

https://stackoverflow.com/questions/5129386/how-to-detect-when-history-pushstate-and-history-replacestate-are-used/25673911#25673911

Screenshot of the order of events:
Screen-Shot-2021-03-09-at-8 15 27-AM-blurred

"Router Loading" logs the loading prop and pathname on useAuth within RouterImpl.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for looking in to this @o0charlie0o What you're describing explains a few odd things I've noticed with auth state as well. So I'd love to get to the bottom of this.

15 minutes later...

So I've been digging through the code, and I think I might have a solution. I'm too tired right now to keep everything in my head, so I'll have to actually try it out to know if it'll work or not. I'll try to verify my idea tomorrow. In the meantime, can you help me by describing the absolute simplest way to reproduce this issue in a newly created RW app? Or if you can set up a repo I can clone that shows the problem. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I have cloned your repo now, and reproduced the issue.
Next step for me is to see if I can fix it :)

Copy link
Member

Choose a reason for hiding this comment

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

I've spent some time working on this. I have also talked to @peterp about it. I have a way forward, but it's a lot of work, and restructuring of the RW code. Got stuck on a timing issue inside the auth code. Peter said he would think about it, see if he could come up with a good solution.

Basically I've created a new package, @redwoodjs/history that lifts that stuff higher up, so it can be used from both auth and router.

Unfortunately I don't think I will have time to finish this in the current sprint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tobbe thanks for the update!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi gents, confirming we'll be pausing PR merges for this Sprint by the end-of-day tomorrow. Tomorrow will be focused on the final review and QA for PRs in this Sprint.

I'll move this as a priority for the next Sprint. Also, I'm working on improving our patch release process, which means hopefully we could get this out as a patch as soon as it's ready. TBD

Thanks for keeping this moving forward. Definitely important!

Copy link
Member

Choose a reason for hiding this comment

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

Posted a WIP PR #2212

}

const withToken = setContext(async () => {
if (isAuthenticated && getToken) {
Expand Down