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

Added option to run under subpath/basepath #2383

Closed
wants to merge 4 commits into from

Conversation

megamit
Copy link

@megamit megamit commented Jan 1, 2022

Description

Can now pass in the BASE_PATH environment variable to tell overseerr to host under a sub path.
e.g. BASE_PATH=/request_some_stuff would run as example.com/request_some_stuff/

To-Dos

  • Successful build yarn build

Issues Fixed or Closed

Copy link
Collaborator

@TheCatLady TheCatLady left a comment

Choose a reason for hiding this comment

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

Looks pretty good 👍🏻

next.config.js Outdated Show resolved Hide resolved
src/utils/baseUrl.ts Outdated Show resolved Hide resolved
src/components/Link/index.tsx Show resolved Hide resolved
src/components/ResetPassword/RequestResetLink.tsx Outdated Show resolved Hide resolved
src/components/Settings/SettingsMain.tsx Outdated Show resolved Hide resolved
src/components/Settings/SettingsMain.tsx Outdated Show resolved Hide resolved
src/components/Settings/SettingsMain.tsx Outdated Show resolved Hide resolved
src/components/Settings/SettingsMain.tsx Outdated Show resolved Hide resolved
@TheCatLady
Copy link
Collaborator

Also, have you tested to verify the service worker still functions as expected when a base path is configured?

@megamit
Copy link
Author

megamit commented Jan 3, 2022

Also, have you tested to verify the service worker still functions as expected when a base path is configured?

I completely missed the service worker sorry. (was completely borked. wouldnt register either)
My new commit should fix it.

@TheCatLady
Copy link
Collaborator

I wonder if a wrapper for useRouter that sets the basePath property of the returned router object would work? 🤔 Then it wouldn't be necessary to add addBasePath to every router.push...

@megamit
Copy link
Author

megamit commented Jan 3, 2022

I wonder if a wrapper for useRouter that sets the basePath property of the returned router object would work? 🤔 Then it wouldn't be necessary to add addBasePath to every router.push...

Before doing the router.push/link wrapper i tried various incarnations of trying to monkey patch the basePath variable the router uses but the deeper i dug felt like it would be super hacky and fragile to implement. Internally the next/Link component is wrapper to router push/replace so if were able to patch router it would take care of those too.

Oh do you mean like a useRouter wrapper link the wrapper. Should be possible sure.

@lgtm-com

This comment has been minimized.

@megamit megamit marked this pull request as ready for review January 4, 2022 10:35
@megamit megamit requested a review from sct as a code owner January 4, 2022 10:35
@brettpetch brettpetch mentioned this pull request Feb 13, 2022
6 tasks
@stale
Copy link

stale bot commented Mar 9, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 9, 2022
@stale stale bot closed this Mar 16, 2022
@coder8338
Copy link

@megamit @TheCatLady Is there anything left to be done on this? Happy to help if you let me know what's remaining to get this merged.

@megamit
Copy link
Author

megamit commented Apr 5, 2022

@coder8338 This got stuck as i couldnt do this cleanly without messing with next.js internals using the approach i took.
It was fully working by my last commit (and i've been using it since) but would brittle to any refactoring nextjs could make to how they send rewrite logic to the ui router.
Next js would need to expose what i was attempting to modify as a proper api or this would be unmaintanable.

Most likely you would need to start from scratch on a different approach.

@crashkonijn
Copy link

Hi!

Would I be correct that this topic is about the same thing?

vercel/next.js#16059

Especially this topic (3 days old now) is about what we'd need for this to happen (if I understood it correctly)
vercel/next.js#41769

@danshilm
Copy link
Collaborator

danshilm commented Apr 4, 2023

@crashkonijn Yep that's exactly it

@danshilm danshilm removed the stale label May 1, 2023
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.

Run overseerr on subpath, Base URL
5 participants