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

Jump to bottom

fix: add authenticatedpageroute so learner can create a shared record link #187

Merged
merged 2 commits into from
Sep 29, 2023
Merged

fix: add authenticatedpageroute so learner can create a shared record link #187

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
adcb3bf
fix: add authenticatedpageroute so learner can create a shared record…
jsnwesson Sep 29, 2023
2479538
fix: add authenticated page route to verifiable credentials
jsnwesson Sep 29, 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
6 changes: 3 additions & 3 deletions src/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { Routes, Route } from 'react-router-dom';
import {
APP_INIT_ERROR, APP_READY, subscribe, initialize, mergeConfig, getConfig,
} from '@edx/frontend-platform';
import { AppProvider, ErrorPage } from '@edx/frontend-platform/react';
import { AppProvider, ErrorPage, AuthenticatedPageRoute } from '@edx/frontend-platform/react';
import { HelmetProvider } from 'react-helmet-async';
import Header from '@edx/frontend-component-header';
import Footer from '@edx/frontend-component-footer';
Expand All @@ -29,7 +29,7 @@ subscribe(APP_READY, () => {
<Routes>
<Route
path={ROUTES.PROGRAM_RECORDS}
element={<ProgramRecordsList />}
element={<AuthenticatedPageRoute><ProgramRecordsList /></AuthenticatedPageRoute>}
Copy link
Member

@MaxFrank13 MaxFrank13 Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I was under the impression that the AuthenticatedPageRoute component was actually a react-router Route of its own. This is indicated in the docs, HOWEVER, the implementation used in frontend-platform test files just uses it as a wrapper (like this PR does).

When digging into the code, I don't see anywhere that AuthenticatedPageRoute would ever return an actual Route component. See the code in frontend-platform.

What's strange is that there ARE implementations where MFEs are adding props like path and component to the AuthenticatedPageRoute component. See here in frontend-app-learner-portal-enterprise.

All of this to say, this PR is clearly fine but I'm also clearly a little confused so will be bringing this to fedX group to hopefully get some clarification on how this component is intended to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey thanks for bringing in all of these PR examples! Very valid point in terms of implementation. Originally I did try replacing the Route element with a AuthenticatedPageRoute, but then an error appeared:

[AuthenticatedPageRoute] is not a <Route> component. All component children of <Routes> must be a <Route> or <React.Fragment>

This must be a hard requirement for frontend-platform v5, and previous PRs might need to be updated to reflect this rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One other thing to note is that the comments in the docs is from 4 years ago (found via git blame), so it should be updated to better represent its implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And lastly, it looks like the implementation in frontend-app-learner-portal-enterprise has placed the routes directly into App.jsx (and not wrapped with a Routes element), and this was done over two years ago. And they're still using frontend-platform v4.

So from that, I think that AuthenticatedPageRoute once had served as a substitute for Route, but it no longer does in v5.

/>
{getConfig().ENABLE_VERIFIABLE_CREDENTIALS && (
<Route
Expand All @@ -43,7 +43,7 @@ subscribe(APP_READY, () => {
/>
<Route
path={ROUTES.PROGRAM_RECORD_ITEM}
element={<ProgramRecord isPublic={false} />}
element={<AuthenticatedPageRoute><ProgramRecord isPublic={false} /></AuthenticatedPageRoute>}
/>
</Routes>
) : (
Expand Down