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.

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

Strip ++api++ from in-browser location changes #4834

Closed
wants to merge 1 commit into from

Conversation

JeffersonBledsoe
Copy link
Member

Solves the Volto part of #4800. We're stripping the ++api++ if it is in a location change as we probably don't want to ever navigate directly to a page with ++api++ in it in the browser. Change the action causes the redux store to be correct, but the URL updating occurs earlier in the react-router stack, and so we also manually update the URL to not have ++api++ in it. History is still preserved correctly (++api++ pages aren't in the history stack but we can still go back and forwards normally).

The logic is currently mixed in with the blacklistRoutes middleware while I get some feedback, but this logic should probably be its own middleware for readability.

@netlify
Copy link

netlify bot commented May 31, 2023

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 222e9b6
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/6477225d1eb29b00085e7e04

@JeffersonBledsoe JeffersonBledsoe changed the title Strip ++api++ from location changes Strip ++api++ from in-browser location changes May 31, 2023
@JeffersonBledsoe JeffersonBledsoe marked this pull request as ready for review May 31, 2023 10:49
const { externalRoutes = [] } = config.settings;

const route = externalRoutes.find((route) =>
matchPath(pathname, route.match),
);

let actionToSend = action;
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK here, in Javascript, actionToSend is a reference to action. So removing this assignment, and directly changing the action object above, has the same effect. Otherwise, if the intent was to keep the action object as is, there is a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mamico I agree on the behaviour of actionToSend being a reference and so this should be redundant, however I was experiencing some issues with it using the outdated URL if I didn't take a reference to it first.

Updated PR in #4854

@JeffersonBledsoe
Copy link
Member Author

GitHub bug meant I had to close this PR and re-open it in #4854

@davisagli davisagli mentioned this pull request May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants