fix: page routes wildcard which breaks tutor #322

connorhaugh · 2023-09-26T15:38:27Z

In f62d0aa
we upgraded react router. That change also added the following

<Route path={`${ROUTES.Detail.HOME}`} element={<StudioHeaderWrapper />} />

became

<Route path={`${ROUTES.Detail.HOME}/*`} element={<StudioHeaderWrapper />} />

That broke tutor, as tutor uses /<MFE_TYPE>/.

We know this fix works on tutor and devstack. We want to ensure, however, @Syed-Ali-Abbas-Zaidi that this change was not made for a reason, as we don't want to break an unexpected feature. Can you tell us why you made that change Syed?

ormsbee · 2023-09-26T16:15:04Z

I see the test failures, but don't understand them. Does it shed light on why the wildcard was added?

codecov · 2023-09-26T17:04:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4423a3f) 52.41% compared to head (0952f2b) 52.41%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #322   +/-   ##
=======================================
  Coverage   52.41%   52.41%           
=======================================
  Files          75       75           
  Lines        1986     1986           
  Branches      359      359           
=======================================
  Hits         1041     1041           
  Misses        912      912           
  Partials       33       33

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

abdullahwaheed · 2023-09-26T17:09:41Z

Hi @connorhaugh, @Syed-Ali-Abbas-Zaidi is on leaves this week. Actually this route wasn't using exact previously and react-router-v6 have all the exact routes by default, so we have append * in this route so if it has any child routes, they can also work. If its causing issue, we can remove it

abdullahwaheed · 2023-09-26T17:10:18Z

src/index.jsx

@@ -52,7 +52,7 @@ subscribe(APP_READY, () => {
          <Route path={ROUTES.Detail.EDIT} element={<LibraryEditPage />} />
          <Route path={ROUTES.Detail.ACCESS} element={<LibraryAccessPage />} />
          <Route path={ROUTES.Detail.IMPORT} element={<CourseImportPage />} />
-          <Route path={`${ROUTES.Block.HOME}/*`} element={<LibraryBlockPage />} />
+          <Route path={`${ROUTES.Block.HOME}`} element={<LibraryBlockPage />} />


don't revert this one, this route has child routes

@ormsbee any chance you could test this out on Tutor with the wildcard in this route?

I actually get the same error either way. Not sure if I'm doing something wrong. 😦

react.development.js:220 Warning: Failed prop type: The prop `library` is marked as required in `StudioHeaderWrapperBase`, but its value is `null`. at StudioHeaderWrapperBase (webpack-internal:///./src/library-authoring/studio-header-wrapper/StudioHeaderWrapper.jsx:53:7) at ShimmedIntlComponent (webpack-internal:///./node_modules/@edx/frontend-platform/i18n/injectIntlWithShim.js:151:7) at injectIntl(ShimmedIntlComponent) at ConnectFunction (webpack-internal:///./node_modules/react-redux/es/components/connectAdvanced.js:233:68) at RenderedRoute (webpack-internal:///./node_modules/react-router/dist/index.js:554:5) at Routes (webpack-internal:///./node_modules/react-router/dist/index.js:1185:5) at Router (webpack-internal:///./node_modules/react-router/dist/index.js:1123:15) at BrowserRouter (webpack-internal:///./node_modules/react-router-dom/dist/index.js:399:5) at Provider (webpack-internal:///./node_modules/react-redux/es/components/Provider.js:18:20) at OptionalReduxProvider (webpack-internal:///./node_modules/@edx/frontend-platform/react/OptionalReduxProvider.js:18:20) at ErrorBoundary (webpack-internal:///./node_modules/@edx/frontend-platform/react/ErrorBoundary.js:139:5) at IntlProvider (webpack-internal:///./node_modules/react-intl/lib/src/components/provider.js:92:47) at AppProvider (webpack-internal:///./node_modules/@edx/frontend-platform/react/AppProvider.js:108:20)

Okay, so that is a warning, and if I hardcode the path for LibraryListPage to be "library-authoring", it does render–even though that warning still spills out in the console.

I'm not sure. It just seems very odd that react-router would overwrite that instead of appending.

Based on the change in how links are handled I thought it might be in https://github.com/openedx/frontend-app-library-authoring/blob/master/src/library-authoring/utils/hoc.jsx but nothing there stood out to me.

Maybe @arbrandes has some insight?

I don't, but perhaps @Syed-Ali-Abbas-Zaidi has sufficient context. The router-v6 changes were pretty extensive!

That they were! To add context, I was specifically looking into why the link Dave mentioned was behaving differently after changing from

goToCreateLibraryPage = () => { this.props.history.push(ROUTES.List.CREATE); };

to

goToCreateLibraryPage = () => { this.props.navigate(ROUTES.List.CREATE); };

which is what got me looking into hoc.jsx

Questions:
Did you restart/rebuild after making the changes?
Just to confirm, we never tested the problem as fixed on your local tutor?

I did restart/rebuild between changes. Going back to the commit before the react-router upgrade (so commit 16414ff) works. Grabbing the latest master fails. Grabbing this branch also fails in the same way master is failing.

src/index.jsx

brian-smith-tcril · 2023-09-26T17:12:24Z

src/index.jsx

@@ -52,7 +52,7 @@ subscribe(APP_READY, () => {
          <Route path={ROUTES.Detail.EDIT} element={<LibraryEditPage />} />
          <Route path={ROUTES.Detail.ACCESS} element={<LibraryAccessPage />} />
          <Route path={ROUTES.Detail.IMPORT} element={<CourseImportPage />} />
-          <Route path={`${ROUTES.Block.HOME}/*`} element={<LibraryBlockPage />} />
+          <Route path={`${ROUTES.Block.HOME}`} element={<LibraryBlockPage />} />


Suggested change

<Route path={`${ROUTES.Block.HOME}`} element={<LibraryBlockPage />} />

<Route path={ROUTES.Block.HOME} element={<LibraryBlockPage />} />

Fair. Going to wait for Dave's test then I'll put up an amended version.

ormsbee · 2023-10-30T05:21:58Z

src/index.jsx

@@ -52,7 +52,7 @@ subscribe(APP_READY, () => {
          <Route path={ROUTES.Detail.EDIT} element={<LibraryEditPage />} />
          <Route path={ROUTES.Detail.ACCESS} element={<LibraryAccessPage />} />
          <Route path={ROUTES.Detail.IMPORT} element={<CourseImportPage />} />
-          <Route path={`${ROUTES.Block.HOME}/*`} element={<LibraryBlockPage />} />


@kdmccormick: I lost track of this when openedx/frontend-platform#574 seemed to make things work on tutor. But I'm wondering if this could be the cause of the component edit page still being broken on Tutor dev?

Co-authored-by: Brian Smith <112954497+brian-smith-tcril@users.noreply.github.com>

…router-upgrade

navinkarkera · 2024-03-11T12:24:43Z

@connorhaugh Gentle reminder.

brian-smith-tcril · 2024-03-29T16:33:02Z

@ormsbee @connorhaugh is this PR still needed?

openedx/frontend-platform#574 was merged which made it into https://github.com/openedx/frontend-platform/releases/tag/v5.5.1 and the package.json on this repo specifies 5.6.1

https://github.com/openedx/frontend-app-library-authoring/blob/7303f79fd6e8ad64270ad419d3507aeb588308b2/package.json#L43

kdmccormick · 2024-07-17T19:50:35Z

Closing per #439

connorhaugh requested a review from a team as a code owner September 26, 2023 15:38

connorhaugh requested review from ormsbee, arbrandes, brian-smith-tcril and Syed-Ali-Abbas-Zaidi September 26, 2023 15:38

fix: page routes wildcard which breaks tutor

44195fb

jesperhodge force-pushed the fix--tutor-page-routes-broken-after-react-router-upgrade branch from 18ac379 to 44195fb Compare September 26, 2023 16:58

abdullahwaheed reviewed Sep 26, 2023

View reviewed changes

brian-smith-tcril reviewed Sep 26, 2023

View reviewed changes

ormsbee reviewed Oct 30, 2023

View reviewed changes

kdmccormick and others added 2 commits February 2, 2024 11:24

style: redundant interpolation

e36f1da

Co-authored-by: Brian Smith <112954497+brian-smith-tcril@users.noreply.github.com>

Merge branch 'master' into fix--tutor-page-routes-broken-after-react-…

0952f2b

…router-upgrade

kdmccormick closed this Jul 17, 2024

kdmccormick deleted the fix--tutor-page-routes-broken-after-react-router-upgrade branch July 17, 2024 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: page routes wildcard which breaks tutor #322

fix: page routes wildcard which breaks tutor #322

connorhaugh commented Sep 26, 2023

ormsbee commented Sep 26, 2023

codecov bot commented Sep 26, 2023 •

edited

Loading

abdullahwaheed commented Sep 26, 2023

abdullahwaheed Sep 26, 2023

brian-smith-tcril Sep 26, 2023

ormsbee Sep 26, 2023

ormsbee Sep 26, 2023

ormsbee Sep 26, 2023 •

edited

Loading

brian-smith-tcril Sep 27, 2023

arbrandes Sep 27, 2023

brian-smith-tcril Sep 27, 2023

connorhaugh Sep 27, 2023 •

edited

Loading

ormsbee Sep 28, 2023

brian-smith-tcril Sep 26, 2023

connorhaugh Sep 26, 2023

ormsbee Oct 30, 2023 •

edited

Loading

navinkarkera commented Mar 11, 2024

brian-smith-tcril commented Mar 29, 2024

kdmccormick commented Jul 17, 2024

	<Route path={`${ROUTES.Block.HOME}`} element={<LibraryBlockPage />} />
	<Route path={ROUTES.Block.HOME} element={<LibraryBlockPage />} />

fix: page routes wildcard which breaks tutor #322

fix: page routes wildcard which breaks tutor #322

Conversation

connorhaugh commented Sep 26, 2023

ormsbee commented Sep 26, 2023

codecov bot commented Sep 26, 2023 • edited Loading

Codecov Report

abdullahwaheed commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorhaugh Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

navinkarkera commented Mar 11, 2024

brian-smith-tcril commented Mar 29, 2024

kdmccormick commented Jul 17, 2024

codecov bot commented Sep 26, 2023 •

edited

Loading

ormsbee Sep 26, 2023 •

edited

Loading

connorhaugh Sep 27, 2023 •

edited

Loading

ormsbee Oct 30, 2023 •

edited

Loading