Skip to content

Allow passing a user-defined client#134

Closed
a-type wants to merge 1 commit intoauth0:masterfrom
a-type:user-provided-client
Closed

Allow passing a user-defined client#134
a-type wants to merge 1 commit intoauth0:masterfrom
a-type:user-provided-client

Conversation

@a-type
Copy link

@a-type a-type commented Oct 17, 2020

Description

This PR adds the option for a user to provide their own instantiated Auth0Client to Auth0Provider. Enabling this behavior allows users to manage an instance of the client outside of the React lifecycle and utilize it in non-React code without awkward or performance-impacting workarounds. This unlocks a far more streamlined path to integration with the most popular React API-integration libraries, like Apollo, Relay, react-query, SWR, and more.

An example app has been added to demonstrate a simple use case by integrating Auth0 with the popular react-query library and defining a custom "default query function" which automatically authorizes the request. While a static client would not be necessary to use react-query in particular, it does demonstrate the far more streamlined nature of usage for the library enabled by this change. Other more complicated setups, as with Apollo, are far more reliant on this change not just for convenience, but for pure compatibility.

Changes

This PR does not intentionally* introduce breaking changes, but it does expand the library API in two ways:

  1. Allow users to pass a client prop to Auth0Provider. This client prop is exclusive with most of the other configuration props (as it would not be possible to re-configure the instantiated client). Thus, the TypeScript prop typings have been updated to reflect this mutual exclusivity and reject props that contain both client and any other configuration properties.
  2. Expose Auth0Client and Auth0ClientOptions from the module, as exported from auth0-spa-js. Because auth0-react relies on auth0-spa-js as a dependency, this was a decision between exposing them or moving auth0-spa-js to a peerDependency, to avoid version mismatch. I considered exposing them to be both more convenient and less disruptive to existing library users.

* There is, in fact, some small potential this could result in breakage for certain users, as the original prop typings allowed [key: string]: any within the interface, and a user could potentially have defined their own client prop with a different meaning. This behavior is up for discussion, as I'm not sure of the actual use cases surrounding arbitrary props.

References

This PR addresses user requests or problems as reflected in:

I am aware that library maintainers have expressed an unwillingness to allow this pattern of usage, and proposed various workarounds. However, I have yet to find any of them acceptable with respect to the larger community consensus on React best practices.

Alternative: Creating a client instance during render

This is the latest recommendation (per #105, #97), however it is a novel approach which carries significant risk of the Auth0-dependent service (in this case, Apollo Client) being unexpectedly re-instantiated, or of multiple copies existing at once (for example, in the upcoming React Concurrent Mode). As React users, we have no control and little actual insight into React's lifecycle management decisions, which are not guaranteed as part of its public API. Any computation performed within a render should be pure and ready to run at any time without side-effects..

In order to adapt to this restriction, we would have to memoize the instantiation of the Apollo Client based on the value of getAccessTokenSilently - but this is currently impossible as the function is not a stable reference. Even if it were, though, users would be tempted to utilize React's useMemo for this purpose - but like the rest of React's performance details useMemo is not guaranteed to strictly memoize, and its implementation may be changed at any time and should not be relied upon to generate stable references.

Alternative: Extracting the client to a module-scoped variable

This is definitely a novel hack I haven't seen proposed elsewhere in the React community. It introduces the mental overhead of the concern about whether or not the module-scope variable has been defined yet - which will only happen when React has decided to render a component. It also relies on side effects in a React render (setting a singleton variable), which as with the previous approach, is not recommended or supported by React and liable to become unstable with future releases.

Prior art for the provided-client approach

The pattern of providing a statically-defined "client" object (or, more broadly, any singleton instance of a class which represents the integral state management logic for the functionality provided to the React tree) is well-known and supported by nearly every popular React integration for tools where such a pattern is applicable:

Concerns about client state management

Reasons for not approving of the pattern proposed in this PR largely revolve around concerns about client state management. I have largely opted not to try to address that concern directly in the code as written. Since this proposed pattern is specifically an advanced pattern, users who utilize it should familiarize themselves with the workings of auth0-spa-js and be aware of their own application's lifecycle.

For what it's worth, I saw no potential issues even with my very minimal and simple implementation of this feature. Even if a user needed to initialize their client before rendering the React application by calling checkSession, etc - the token cache should simply be populated by the time React renders, which appears to be idempotent and not dangerous to the behavior of the library.

Testing

Describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

Please include any manual steps for testing end-to-end or functionality not covered by unit/integration tests.

Also include details of the environment this PR was developed in (language/platform/browser version).

  • This change adds test coverage for new/changed/fixed functionality
  • Maintainers can test the new pattern using the provided example app

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@a-type a-type requested a review from a team October 17, 2020 17:44
@Maxime-Fleurant
Copy link

why is there any information about those request, it's been a week that i'm looking for a way to implement auth0 static strategy with next and apollo.

@adamjmcgrath
Copy link
Contributor

Hi @a-type

Thanks for your PR. Currently the advice is still to use the recommendation in #97 (comment)

When I have some time next week, I'll go through your concerns about this approach

@adamjmcgrath
Copy link
Contributor

Hi @a-type - thanks again for your work on this PR

Unfortunately, I've decided not to accept it, and I'll list the reasons below:

  1. Your example creates an instance of the client on the module scope that would leak state between tests and, in an isomorphic app, between requests - I know you dismissed concerns about client state management, but it's still not a use case we'd like to facilitate if we can avoid it.
  2. Your example shows a use case where one provider depends on the other, and could be resolved fairly simply by passing the output of the Auth0Provider (the getAccessTokenSilently method) into the ReactQueryCacheProvider, for example:
ReactDOM.render(
  <React.StrictMode>
    <Auth0Provider ...>
      {() => {
        const { getAccessTokenSilently } = useAuth0();
        return (
          <ReactQueryCacheProvider queryCache={() => ... use getAccessTokenSilently... }>
            <App />
          </ReactQueryCacheProvider>)
      }}
    </Auth0Provider>
  </React.StrictMode>,
  document.getElementById('root')
);
  1. auth0-react's getAccessTokenSilently (and loginWithPopup) behaves differently to client.getTokenSilently (it throws different errors) and it will diverge more when user is not updated after getAccessTokenSilently({ignoreCache: true}) #109 is fixed. We may want to continue to diverge the client's behaviour from the react hooks behaviour, even possibly to remove the dependence on the client all together if the need arises.

Ultimately, while I appreciate that it could be used as an advanced feature, and that it would make use cases like yours slightly easier, I've not seen a use case where it's not reasonable to use the SDK's hook as designed. But, be aware that this recommendation is constantly under review and I'm very happy to look at more use cases and discuss further.

@a-type
Copy link
Author

a-type commented Oct 28, 2020

Thanks for considering it. Since I've opted not to use Auth0 for the moment I'll probably leave it there.

@richardscarrott
Copy link

richardscarrott commented May 10, 2021

Hi @a-type - thanks again for your work on this PR

Unfortunately, I've decided not to accept it, and I'll list the reasons below:

  1. Your example creates an instance of the client on the module scope that would leak state between tests and, in an isomorphic app, between requests - I know you dismissed concerns about client state management, but it's still not a use case we'd like to facilitate if we can avoid it.
  2. Your example shows a use case where one provider depends on the other, and could be resolved fairly simply by passing the output of the Auth0Provider (the getAccessTokenSilently method) into the ReactQueryCacheProvider, for example:
ReactDOM.render(
  <React.StrictMode>
    <Auth0Provider ...>
      {() => {
        const { getAccessTokenSilently } = useAuth0();
        return (
          <ReactQueryCacheProvider queryCache={() => ... use getAccessTokenSilently... }>
            <App />
          </ReactQueryCacheProvider>)
      }}
    </Auth0Provider>
  </React.StrictMode>,
  document.getElementById('root')
);

Does Auth0Provider support children as a render prop, the TS types don't seem to allow it? Also, I don't think it's safe to call a hook useAuth0 inside a render prop unless auth0-react is supporting an unusual guarantee of never conditionally rendering?

I'd love to see a real world example of how to get hold of the access token in Apollo Client as this feels so brittle and clunky at the moment.

@richardscarrott
Copy link

richardscarrott commented May 10, 2021

Also, with that example we then lose the ability to use the apollo client outside of react render 🤔

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.

4 participants