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

fix: remove trailing slash from PUBLIC_PATH env var #154

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

ormsbee
Copy link
Contributor

@ormsbee ormsbee commented Oct 3, 2023

We previously generated the PUBLIC_PATH base for each MFE with a trailing slash, e.g. "/authn/". But the actual links that point to these from the LMS and Studio don't have a trailing slash in them, e.g. "/authn". This commit removes the trailing slash to be consistent.

@ormsbee
Copy link
Contributor Author

ormsbee commented Oct 3, 2023

FYI @brian-smith-tcril, @arbrandes

@regisb
Copy link
Contributor

regisb commented Oct 3, 2023

Hi Dave! Thanks for the PR. Does this change fix a particular issue, or is it just a cosmetic change?

@brian-smith-tcril
Copy link
Contributor

brian-smith-tcril commented Oct 3, 2023

@regisb if this change gets in it should remove the need to add extra trailing slash handling logic in frontend-platform (which is now needed due to changes in React Router v6). The discussion on openedx/frontend-platform#574 can provide more context.

@regisb
Copy link
Contributor

regisb commented Oct 3, 2023

I have no idea what's going on and I should never have asked 😅 passing it on to you @arbrandes! I'd just like to ask that you create a changelog entry with scriv create and to explain in that entry why the change was made (as opposed to what was made). For instance "make it easier and consistent for frontend-patform to parse the PUBLIC_PATH" could be a perfectly reasonable changelog description.

We previously generated the PUBLIC_PATH base for each MFE with a
trailing slash, e.g. "/authn/". But the actual links that point to
these from the LMS and Studio don't have a trailing slash in them,
e.g. "/authn". This commit removes the trailing slash to be consistent.
Removing that trailing slash in configuration also means that we don't
have to strip it out on the JavaScript side of things, which would have
been necessary after the React Router 6 upgrade (we send it as the
router's basename).
Copy link
Collaborator

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks, Dave!

@arbrandes arbrandes merged commit ae60b2d into overhangio:nightly Oct 3, 2023
@ormsbee ormsbee deleted the remove-trailing-slash branch October 3, 2023 19:57
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.

4 participants