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

feat(remix-dev): add serverBasename config option #5236

Closed
wants to merge 2 commits into from

Conversation

yracnet
Copy link

@yracnet yracnet commented Jan 24, 2023

I have implemented the feature described on the #2891.

This include the serverBasename attribute on remix.config.js for declare a prefix to URL

serverBasename default value is ""
fix deploy on path server (basename)
fix the navegation in client side or server side components

clean branch from from #4459

@changeset-bot
Copy link

changeset-bot bot commented Jan 24, 2023

⚠️ No Changeset found

Latest commit: dd985cf

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

@yracnet yracnet mentioned this pull request Jan 24, 2023
@kentcdodds
Copy link
Member

kentcdodds commented Jan 29, 2023

Thanks a lot! This would definitely be a good feature to have.

Could we get an integration test for this please?

@justinwaite
Copy link
Contributor

justinwaite commented Jan 29, 2023

Something I've struggled with Remix apps hosted under the same domain at my company is redirecting from loaders and actions to other apps. For example, say we have the following apps:

App 1: domain.com/app1
App 2: domain.com/app2
... etc.
Launchpad: domain.com/home

Now say that in App 2 I return a response that should redirect to /home. What happens currently is that remix tries to find the /home route within the App2 remix router manifest, does not find it, then renders a 404.

This makes it difficult to do something like redirecting to a login page that lives in the "Launchpad" app. This PR should also handle this case if it doesn't. I know that the redirect logic within remix does handle external (non-remix) routes, but only if the domain is different. Now I want remix to check the domain + this serverBasepath.

I'm unsure if this logic lives in Remix or React Router right now, but I'm assuming Remix, since it's dependent on the X-remix-redirect header.

@justinwaite
Copy link
Contributor

I think I've discovered that the redirect issue will need to be resolved in react-router. I'll try and put together a PR over there.

@maxprilutskiy
Copy link
Contributor

@justinwaite @kentcdodds @yracnet folks, any chance we can solve this any time soon? Any support needed with anything?

@justinwaite have you put together a PR you talked about or not just yet?

@justinwaite
Copy link
Contributor

@justinwaite @kentcdodds @yracnet folks, any chance we can solve this any time soon? Any support needed with anything?

@justinwaite have you put together a PR you talked about or not just yet?

The redirect and Link changes should be tied to this bug: remix-run/react-router#10052

Looks like these will be taken care of soon. Then Remix needs to be updated to allow a base name.

@MichaelDeBoey MichaelDeBoey changed the title Feature/basename r2 feat(remix-dev): add serverBasename config option Mar 9, 2023
@19Qingfeng
Copy link

I want to know what happened to this pr.

@dmbarry86
Copy link

What's the status with this PR? When can we expect this to land? We're about to implement something similar in our remix app suite using https://github.com/kiliman/remix-mount-routes but if this feature is about to land in remix itself then that would be my preferred option.

@rolandjohann
Copy link

We need this feature, too and investigated into implementing it ourself. If this PR is stale, how is the process to take over the work? At least @dmbarry86 mentioned implementing the desired functionality via third party libs, we can invest into it as well.

@MichaelDeBoey
Copy link
Member

Hi @yracnet!

Are you still interested in getting this one merged?

If so, please rebase onto latest dev & resolve conflicts

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 2, 2023
@github-actions
Copy link
Contributor

This PR has been automatically closed because we haven't received a response from the original author 🙈. This automation helps keep the issue tracker clean from PRs that are unactionable. Please reach out if you have more information for us! 🙂

@github-actions github-actions bot closed this May 12, 2023
@yracnet
Copy link
Author

yracnet commented Jun 1, 2023

Hi, I have implemented a plugin in ViteJS that provides support for the server's basename. You can check it out at https://www.npmjs.com/package/vite-plugin-remix

@maximilize
Copy link

Sad to see this PR gone, would be a helpful feature. What's the reason for not doing it the remix way, but to use vite?

@silvenon
Copy link
Contributor

silvenon commented Nov 21, 2023

The Vite plugin seems to be using Remix internals to hack around this. I would've love to see Remix support for this because it uses React Router under the hood, which supports the basename prop, so I'm a little skeptical that the base path problem can be solved cleanly outside of Remix.

EDIT: Here I'm referring to vite-plugin-remix mentioned in #5236 (comment).

Vite itself has a base option, but it only refers to assets. Perhaps now that Remix is moving to Vite that option can be used in the Remix Vite plugin somehow and used accordingly.

@justinwaite
Copy link
Contributor

The Vite plugin seems to be using Remix internals to hack around this. I would've love to see Remix support for this because it uses React Router under the hood, which supports the basename prop, so I'm a little skeptical that the base path problem can be solved cleanly outside of Remix.

Vite itself has a base option, but it only refers to assets. Perhaps now that Remix is moving to Vite that option can be used in the Remix Vite plugin somehow and used accordingly.

This is what I would like to see as well, but perhaps we need to wait for vite support to stabilize more. I know that some changes are occurring with the asset paths per #8077

@0xahmedk
Copy link

0xahmedk commented Jan 3, 2024

Is anyone still interested in getting this PR merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed needs-response We need a response from the original author about this issue/PR NEW API package:dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet