Skip to content

fix: respect basename in loaders and actions redirects #9418

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
Merged
merged 1 commit into from
Oct 11, 2022
Merged

fix: respect basename in loaders and actions redirects #9418

merged 1 commit into from
Oct 11, 2022

Conversation

@manzano78
Copy link
Contributor

@manzano78 manzano78 commented Oct 6, 2022

Closes #9417

@changeset-bot
Copy link

changeset-bot bot commented Oct 6, 2022 •
edited
Loading

⚠️ No Changeset found

Latest commit: 9cf2cb0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Oct 6, 2022

Hi @manzano78,

Welcome, and thank you for contributing to React Router!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

@manzano78 manzano78 force-pushed the fix/loaders_actions_basename_redirect branch from ce662a2 to 0c6860d Compare October 6, 2022 19:26
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Oct 6, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@brophdawg11
Copy link
Contributor

brophdawg11 commented Oct 6, 2022

Thanks for the PR @manzano78! Do you want to try adding a unit test for this to the router-test.ts file? If not I can get one added 😄

@manzano78
Copy link
Contributor Author

manzano78 commented Oct 6, 2022

Thanks for the PR @manzano78! Do you want to try adding a unit test for this to the router-test.ts file? If not I can get one added 😄

In view of this test file, I think it'd probably be better (for my brain and for the conservation of your strict testing conventions) if you add it 😄 I hope you don't mind!

@manzano78 manzano78 force-pushed the fix/loaders_actions_basename_redirect branch 3 times, most recently from fdee293 to 9a78c3e Compare October 7, 2022 05:26
@manzano78
Copy link
Contributor Author

manzano78 commented Oct 7, 2022 •
edited
Loading

@brophdawg11 , FYI the bundles size test failed because packages/router/dist/router.js exceeded 100kb. As a "workaround" to go back under this threshold, I shortened my code explanation comment. I feel it could be a good solution to increase a little bit this threshold or maybe, if it's possible, to exclude comments from the bundle size calculation to avoid shortening comments as I did :)

@manzano78 manzano78 force-pushed the fix/loaders_actions_basename_redirect branch from 9a78c3e to 92eef91 Compare October 7, 2022 08:05
@manzano78 manzano78 force-pushed the fix/loaders_actions_basename_redirect branch from 92eef91 to 9cf2cb0 Compare October 7, 2022 08:10
@manzano78 manzano78 changed the title fix(router): respect basename in loaders and actions redirects fix: respect basename in loaders and actions redirects Oct 7, 2022
@brophdawg11 brophdawg11 changed the base branch from dev to brophdawg11/redirect-basename October 11, 2022 20:38
@brophdawg11
Copy link
Contributor

brophdawg11 commented Oct 11, 2022

👍 Thanks @manzano78, I'll merge this into a branch of mine and get the tests added there

@brophdawg11 brophdawg11 merged commit b772d47 into remix-run:brophdawg11/redirect-basename Oct 11, 2022
@brophdawg11 brophdawg11 mentioned this pull request Oct 12, 2022
Merged
brophdawg11 added a commit that referenced this pull request Oct 21, 2022
@brophdawg11 @manzano78 @meetic-mikaelanzano
fix: support basename and relative routing in loader/action redirects (…
8f684eb 
…#9447)

* fix: respect basename in loaders and actions redirects (#9418)

Co-authored-by: Mikaël ANZANO <[email protected]>

* fix: support basename and relative routes in redirects

* ci: add tests for data memory router

* add changeset

* Bump bundle threshold

* convert invariant to 404 for missing routeId

* Bundle bump

Co-authored-by: Mikaël Anzano <[email protected]>
Co-authored-by: Mikaël ANZANO <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@manzano78 @brophdawg11 @meetic-mikaelanzano