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

[ormsbee]

Description:

As part of the React Router v5 -> v6 upgrade, history was no longer being passed in. This broke the Tutor deployment method of MFEs, which puts all the MFEs in a common server and uses different top-level dirs to separate them.

The fix for this is to pass in an explicit basename to the Router.

Merge checklist:

• Consider running your code modifications in the included example app within frontend-platform. This can be done by running npm start and opening http://localhost:8080.
• Consider testing your code modifications in another local micro-frontend using local aliases configured via the module.config.js file in frontend-build.
• Verify your commit title/body conforms to the conventional commits format (e.g., fix, feat) and is appropriate for your code change. Consider whether your code is a breaking change, and modify your commit accordingly.

Post merge:

• After the build finishes for the merged commit, verify the new release has been pushed to NPM.

[ormsbee]

@brian-smith-tcril, @arbrandes: I'm not sure what needs to be done in terms of tests and version bumping here?

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (8182d21) 83.17% compared to head (64f3539) 83.19%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #574      +/-   ##
==========================================
+ Coverage   83.17%   83.19%   +0.01%     
==========================================
  Files          40       40              
  Lines        1070     1071       +1     
  Branches      195      195              
==========================================
+ Hits          890      891       +1     
  Misses        168      168              
  Partials       12       12              

Files	Coverage Δ
src/initialize.js	98.79% <100.00%> (+0.01%)	⬆️
src/react/AppProvider.jsx	83.33% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ormsbee]

FYI @kdmccormick

[adamstankiewicz]

Aside from one nit/suggestion, these changes LGTM. I tested using frontend-template-application's MFE on top of devstack with the following test cases:

• PUBLIC_PATH configured as /.
• PUBLIC_PATH configured as /testing.
• PUBLIC_PATH not configured.

All test cases seem to be working as expected.

[adamstankiewicz]

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;
}

[brian-smith-tcril]

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

[arbrandes]

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

[ormsbee]

@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.

[ormsbee]

@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.

[ormsbee]

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.

[ormsbee]

@brian-smith-tcril, @arbrandes: Modified to just take the config value as-is, and relying on overhangio/tutor-mfe#154 to make sure we're getting the right value.

[arbrandes]

Thanks again, Dave! Looks good to me!