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

fix: make Tutor-MFE config compatible with React Router v6 #574

Merged
merged 2 commits into from
Oct 3, 2023
Merged

fix: make Tutor-MFE config compatible with React Router v6 #574

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f080b2
fix: make Tutor-MFE config compatible with React Router v6
ormsbee Oct 2, 2023
64f3539
fix: remove slash trimming and rely on proper tutor-mfe config
ormsbee Oct 3, 2023
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
20 changes: 20 additions & 0 deletions src/initialize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,26 @@ export const history = (typeof window !== 'undefined')
basename: getPath(getConfig().PUBLIC_PATH),
}) : createMemoryHistory();

/**
* The string basename that is the root directory of this MFE.
*
* In devstack, this should always just return an empty string, because each MFE is in its own
* server/domain.
*
* In Tutor, all MFEs are deployed to a common server, each under a different top-level directory.
* The basename is the root path for a given MFE, e.g. "/library-authoring". It is set by tutor-mfe
* as an ENV variable in the Docker file, and we read it here from that configuration so that it
* can be passed into a Router later.
*
* By convention, our basenames do not have a trailing slash. So it's "/library-authoring", and not
* "/library-authoring/". Our configuration is inconsistent on this, so this function always strips
* off the trailing slash from any value entered in configuration.
*/
export function basename() {
const configBasename = getPath(getConfig().PUBLIC_PATH);
return configBasename.endsWith('/') ? configBasename.slice(0, -1) : configBasename;
Copy link
Member

Choose a reason for hiding this comment

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

Nit/Note: not all MFEs currently have a PUBLIC_PATH configured (e.g., presumably MFEs not officially part of Tutor). That said, things seem to still work as expected in this case as getConfig().PUBLIC_PATH falls back to / if not provided.

In the case where getConfig().PUBLIC_PATH is /, this basename function returns an empty string. This would force react-router's Router component's basename prop to fall back to its own / instead.

While it seems to be fine as is, I'm wondering if we should be explicitly handle the fallback to / on our own here without relying on the fallback within Router itself? E.g.

export function basename() {
  const configBasename = getPath(getConfig().PUBLIC_PATH);
  if (configBasename === '/') {
    return configBasename;
  }
  return configBasename.endsWith('/') ? configBasename.slice(0, -1) : configBasename;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do the / stripping at all? Based on https://github.com/remix-run/react-router/blob/f9b3dbd9cbf513366c456b33d95227f42f36da63/packages/router/utils.ts#L1031-L1053

// We want to leave trailing slash behavior in the user's control, so if they
// specify a basename with a trailing slash, we should support it

Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid stripping the tail / here, since:

  1. It's not necessary for react-router (as Brian S shows above)
  2. It goes beyond feature parity with what worked before the react-router-v6 upgrade

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamstankiewicz: I'll make it really explicit in that way, thank you.

@brian-smith-tcril: React Router wants to leave it in our control, but I think that we should be consistent and not have a mix of /library-authoring/, /account, /authn/, etc. Our backend app config is using the ones without trailing slashes, so the current live URLs don't have trailing slashes. So even though I think that having slashes is actually the right thing to do from an academic perspective, I'd rather be standardized on what's already out there.

I do intend to make a separate PR to tutor-mfe to fix how it writes this config, but I thought that for rollout/upgrade ease, we should force this behavior across MFEs.

Copy link
Contributor Author

@ormsbee ormsbee Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@arbrandes, @brian-smith-tcril: Okay, I want to make sure I'm not misunderstanding something.

Given the current state of tutor-mfe-generated config and the URLs we currently have, the code is broken without that stripping. Tutor MFE code will generate the PUBLIC_PATH as /library-authoring/. Trying to load http://apps.local.overhang.io:3001/library-authoring (no trailing slash) will cause an error because the route is not specified that way. The only reason this worked (edit: might have worked?) before was because React Router used to be more permissive about what it would accept here, and in v6 they've gotten more explicit.

I actually have no objection to removing the stripping behavior and relying on everyone getting the updated tutor-mfe generated config. I was just worried that this would complicate things when people are updating, especially if re-deployment doesn't happen all at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the PR to tutor-mfe to fix the config (overhangio/tutor-mfe#154). I'll remove the slash stripping part from this PR.

}

/**
* The default handler for the initialization lifecycle's `initError` phase. Logs the error to the
* LoggingService using `logError`
Expand Down
3 changes: 2 additions & 1 deletion src/react/AppProvider.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
IntlProvider,
LOCALE_CHANGED,
} from '../i18n';
import { basename } from '../initialize';

/**
* A wrapper component for React-based micro-frontends to initialize a number of common data/
Expand Down Expand Up @@ -72,7 +73,7 @@ export default function AppProvider({ store, children, wrapWithRouter }) {
>
<OptionalReduxProvider store={store}>
{wrapWithRouter ? (
<Router>
<Router basename={basename()}>
{children}
</Router>
) : children}
Expand Down