Skip to content

Conversation

@binh-dam-ibigroup
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup commented Sep 18, 2020

This PR replaces the no-longer-maintained use-auth0-hooks library with its successor auth0-react.

This PR also improves the following:

  • Logout redirects to the same server (previously, you could be redirected from e.g. localhost to a live instance).
  • Login from a protected page/route is now supported (e.g. logging-in from /#/account shows the account page instead of the map)

Codewise:

  • Auth0-react renames some props and higher-order components.
  • Auth0-react requires we fetch Auth0 tokens ourselves.
  • Auth0-react inserts some URL parameters, the PR adds code to remove those parameters.
  • appState.returnTo is used where possible (fixes If logging out on local dev environment, does not redirect to localhost:9966 #253), method processSignIn is simplified.
  • Constant URL_ROOT is replaced with the standard window.location.origin. (unchanged, see Use auth0-react #233 (comment))
  • Improved passing props in withLoggedInUserSupport higher order component (removed renderChildrenWithProps util method.
  • state.user.loggedInUserMonitoredTrips now asynchronously fetched after fetching state.user.loggedInUser.

@binh-dam-ibigroup binh-dam-ibigroup marked this pull request as ready for review September 21, 2020 19:05
@binh-dam-ibigroup binh-dam-ibigroup changed the title WIP: Use auth0-react Use auth0-react Sep 21, 2020
Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Haven't fully tested this yet, but it needs a couple of code changes/comments

Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

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

Just one refactoring comment.

@evansiroky evansiroky removed their assignment Sep 22, 2020
Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

See comments from phone discussion in addition to implementing snippet in create/init user:

const isNewAccount = fetchStatus === 'error' || (user && user.result === 'ERR')
const userData = isNewAccount ? createNewUser(authUser) : user
dispatch(setCurrentUser({ accessToken, user: userData }))
if (!isNewAccount) {
  dispatch(fetchUserMonitoredTrips())
}

Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of tiny loose ends.

@binh-dam-ibigroup binh-dam-ibigroup added the BLOCKED Blocked (waiting on another PR to be merged) label Oct 28, 2020
@binh-dam-ibigroup binh-dam-ibigroup removed the BLOCKED Blocked (waiting on another PR to be merged) label Oct 28, 2020
@binh-dam-ibigroup
Copy link
Collaborator Author

FYI @landonreed and @evansiroky there are some not-so-obvious merge changes I addressed in b27eb18. This PR should be ready to merge now.

@binh-dam-ibigroup binh-dam-ibigroup merged commit 98186fd into dev Oct 29, 2020
@binh-dam-ibigroup binh-dam-ibigroup deleted the auth0-react branch October 29, 2020 23:21
@binh-dam-ibigroup binh-dam-ibigroup mentioned this pull request Oct 30, 2020
@landonreed
Copy link
Member

🎉 This PR is included in version 1.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

If logging out on local dev environment, does not redirect to localhost:9966

4 participants