openedx / frontend-platform

A framework for Open edX micro-frontend applications.
http://openedx.github.io/frontend-platform
GNU Affero General Public License v3.0
32 stars 63 forks source link

`getAuthenticatedUser` no longer updates / re-renders after React Router v6 upgrade #591

Open adamstankiewicz opened 11 months ago

adamstankiewicz commented 11 months ago

Description

Seemingly since this PR to upgrade @edx/frontend-platform to React Router v6, components seem to need to be explicitly subscribed to the AppContext when retrieving the authenticated user object within a component as opposed to continuing to rely on getAuthenticatedUser.

This change in behavior resulted in several critical user flows regressing/breaking on production for the frontend-app-learner-portal-enterprise MFE since this PR merged (e.g., infinite loading spinners because the MFE expects getAuthenticatedUser to always represent the full metadata about the user, and it no longer always does). The React Router v6 upgrade was since reverted in this MFE as a result.

Context

The initialize function (entry point for MFEs) allows consumers to optionally hydrateAuthenticatedUser which tells @edx/frontend-platform to make an API call (http://localhost:18000/api/user/v1/accounts/<username>)to retrieve additional metadata about the user (e.g., name, profile image, etc.).

For MFEs, there's generally 2 mechanisms to extract the user's metadata from @edx/frontend-platform, both of which are patterns used by MFEs today:

getAuthenticatedUser

The getAuthenticatedUser function (docs / source) ultimately calls the AxiosJwtAuthService.getAuthenticatedUser function which simply returns whatever this.authenticatedUser it has available.

function MyComponent() {
  const authenticatedUser = getAuthenticatedUser();
  return (
    <div>
      <p>Authenticated Username: <strong>{authenticatedUser.username}</strong></p>
      <p>
        Authenticated user&apos;s name:
        <strong>{authenticatedUser.name}</strong>
        (Only available if user account has been fetched)
      </p>
    </div>
  );
}

AppContext

The AppContext context provider includes (presumably) the same authenticatedUser object as should be returned by getAuthenticatedUser(). However, by subscribing to the AppContext, any time the authenticatedUser changes, the component is guaranteed to re-render with the latest authenticatedUser data.

function MyComponent() {
  const { authenticatedUser } = useContext(AppContext);
  return (
    <div>
      <p>Authenticated Username: <strong>{authenticatedUser.username}</strong></p>
      <p>
        Authenticated user&apos;s name:
        <strong>{authenticatedUser.name}</strong>
        (Only available if user account has been fetched)
      </p>
    </div>
  );
}

Regression

The regression noticed in frontend-app-learner-portal-enterprise and also reproducible in frontend-platform example MFE is that after the React Router v6 upgrade in @edx/frontend-platform, relying on getAuthenticatedUser is not guaranteed to have the correct metadata MFE developers might expect.

When debugging, it was observed that when relying on getAuthenticatedUser before React Router v6 upgrade, the component first renders with the minimal set of user metadata but after a re-render of the component, it then contains the full metadata as expected.

Now, after React Router v6 upgrade, there is only a single call to getAuthenticatedUser returning the minimal user metadata (i.e., missing the hydrated metadata from the API). This seems to be an unexpected change in behavior.

The known workaround is to migrate components away from getAuthenticatedUser() in favor of pulling from AppContext instead to ensure the component is properly re-rendered in order to have the fully hydrated user metadata. That said, an ideal fix would ensure getAuthenticatedUser() always returns the same metadata as returned by AppContext as most consumers would likely expect both mechanisms to have the same behavior, returning the same hydrated user metadata, when used within a component.

Actual

The behavior after React Router 6 upgrade shows a single console.log of the minimal authenticated user metadata. It is missing the full hydrated user metadata including properties like profileImage, etc. until something triggers a re-render of the component.

image

Expected

The behavior before React Router v6 upgrade shows the console.log of the minimal authenticated user metadata first, then an automatic component re-render which then forces another console.log including the full authenticated user metadata instead.

image
adamstankiewicz commented 11 months ago

If pulling authenticatedUser from AppContext is the pattern consumers need to do moving forward post-React Router v6 upgrade, this should ideally be explicitly mentioned in the @edx/frontend-platform GitHub release notes for the React Router v6 upgrade as well as in the React Router v6 upgrade document.

We may also want to improve documentation so consumers have more direction in knowing when to use getAuthenticatedUser vs. pulling authenticatedUser from AppContext.

adamstankiewicz commented 11 months ago

It may also be worth considering why the change in behavior for relying on getAuthenticatedUser() to return the hydrated metadata didn't cause any unit test regressions. For example, if we had any test case(s) that called getAuthenticatedUser within a component and expected the hydrated user metadata (without any mocking), this regression may have more likely been caught earlier.

Syed-Ali-Abbas-Zaidi commented 11 months ago

After thoroughly investigating the issue, we have identified the following behavior:

  1. getAuthenticatedUser() Pre-react-router v6: Prior to react-router v6, when using the getAuthenticatedUser() approach to fetch user's metadata, the below-mentioned code was observed to trigger two separate renderings of the component. The first rendering console logged minimal metadata, and the second rendering console logged complete metadata. This is an unexpected behavior since React typically re-renders a component only when its state changes. And a simple data change in variable should not trigger component re-render.

    function MyComponent() {
    const authenticatedUser = getAuthenticatedUser();
    
    console.log (authenticatedUser);
    
    return (
    <> ... </>;
    );
    }
  2. AppContext: In contrast, when the user's metadata was obtained through the AppContext approach, the component reliably re-rendered with the most up-to-date metadata. The AppContext mechanism, although also reliant on getAuthenticatedUser(), operates by storing the data in a state variable and actively monitors for changes in metadata. When changes are detected, it updates the state accordingly, prompting a re-render of the component.

Our conclusion is that the issue lies not with react-router v6 but with how consumers handle user metadata obtained through getAuthenticatedUser(). To address this issue, we recommend the following refactoring:

  1. When consumers utilize getAuthenticatedUser() to retrieve user metadata, they should assign the data to a state variable.
  2. In addition, consumers should also add this hook that actively monitors changes in metadata. When changes occur, this hook should update the associated state variable.

By following these steps, the component will correctly re-render in response to changes in user metadata.

adamstankiewicz commented 10 months ago

Thanks for further investigating, @Syed-Ali-Abbas-Zaidi!

This is an unexpected behavior since React typically re-renders a component only when its state changes. And a simple data change in variable should not trigger component re-render.

I'm still curious why upgrading to React Router v6 is what triggered this change in behavior with re-rendering (e.g., are there any other impacted functions?). Though, I agree we should be relying on AppContext (preferable) or useAppEvent(AUTHENTICATED_USER_CHANGED) hook moving forward.

For the purpose of this issue, we may want to consider whether there's any documentation improvements that could be made so consumers are more readily aware that they shouldn't rely on getAuthenticatedUser within React components if they expect it to contain hydrated user metadata. I'm not sure the current docs around the functions exposed by @edx/frontend-platform make this as clear as it could be for consumers. For example, there's no mention of it here or here.

From the 2nd link above, about the hydrateAuthenticatedUser option on the initialize function:

If true, makes an API call to the user account endpoint (${App.config.LMS_BASE_URL}/api/user/v1/accounts/${username}) to fetch detailed account information for the authenticated user. This data is merged into the return value of getAuthenticatedUser, overriding any duplicate keys that already exist. Defaults to false, meaning that no additional account information will be loaded.

This documentation would lead consumers to believe the hydrated user metadata should always be returned by getAuthenticatedUser, when that is no longer true.

There is also a semi-related comment to this effect at the following link but consumers aren't often looking at the internals of how @edx/frontend-platform works: https://github.com/openedx/frontend-platform/blob/081a269b7ba9b5ca13add94b8374d9dbbe200047/src/initialize.js#L148-L154

I feel it'd might be a good exercise to ensure the documentation around getAuthenticatedUser explicitly mentions when it should be used vs. pulling the authenticatedUser metadata from AppContext, or otherwise.