Mount HTML serving middlewares at config.base #91
Conversation
rmhaiderali
changed the title
|
Hi @haider53e, sorry for such a late response. Could you elaborate a little bit more on the problem you are trying to solve? I want to know about the context in which you stumbled upon this problem, as this will let me understand better why current behaviour is incorrect. |
|
Hi @szymmis, No worries about the delay, and I appreciate your response! The primary objective of this pull request is to align with how the Vite development server mounts applications. Currently, when the base is specified in Vite, it mounts the application at the specified base. However, Vite Express, in its current state, skips the base, and this pull request aims to provide consistency by allowing users to decide whether to skip or include the base. For clarification, if the base is specified: In Vite: The application is mounted at the specified base. Here's a simple example to illustrate the impact: Without this PR, Vite Express would mount the application at http://localhost:3000/, whereas with this PR, it can be configured to mount at http://localhost:3000/my-subdirectory/ |
There was a problem hiding this comment.
Thanks a lot for clarification, now I see the issue.
There is a config.base value but it is currently used only in the injectStaticMiddleware function. I think we can just use it in the both functions that you modified instead of introducing another config option, as I said in the comment.
Also, great catch with the req.method.
src/main.ts
Outdated
| @@ -213,7 +214,9 @@ async function injectViteIndexMiddleware( | |||
| ) { | |||
| const config = await getViteConfig(); | |||
|
|
|||
| app.get("/*", async (req, res, next) => { | |||
| app.use(Config.ignoreBase ? "/" : config.base, async (req, res, next) => { | |||
There was a problem hiding this comment.
| app.use(Config.ignoreBase ? "/" : config.base, async (req, res, next) => { | |
| app.use(config.base, async (req, res, next) => { |
I think we can use config.base directly without introducing another value as it might be confusing. config.base was added to align vite-express with how Vite works with it, but as you noticed, it isn't used here, only in injectStaticMiddleware function. Also it's / by default that's why we dont need to check if its defined or not.
There was a problem hiding this comment.
Sure, using base directly in both functions will be more concise and align Vite Express with Vite.
tests/templates.test.ts
Outdated
| @@ -64,7 +64,7 @@ const testCase = async (template: string, done: () => void) => { | |||
|
|
|||
| browser.then(async (browser) => { | |||
| const page = await browser.newPage(); | |||
| await page.goto("http://localhost:3000"); | |||
| await page.goto("http://localhost:3000/admin"); | |||
There was a problem hiding this comment.
Almost got there 😅, what I suggest is to add base parameter to testCase function, that's set to / by default.
const testCase = async (template: string, done: () => void, base = "/") => {| await page.goto("http://localhost:3000/admin"); | |
| await page.goto(`http://localhost:3000${base}`); |
That's because right now other test would fail (Well, maybe they wouldn't fail because if vite-express is mounted at root, it will serve the page at /admin, but it will definitely make things unclear). If you want to run them locally you can use npm run test or yarn test or any other package manager.
By specifying the parameter as default, you don't need to adjust other testCase invocations, only the one in Template "${template}" with custom inline config test here.
There was a problem hiding this comment.
I've made modifications to the code, but on windows, even after attempting to install with both npm and yarn, I consistently encounter the following error when running npm run test with or without administrative privileges:
TEST Template "react"
FAIL Error: EACCES: permission denied, scandir 'D:\vite-express\create-vite-express\templates\react\node_modules\.bin\browserslist'
FAIL 18 failed, 0 passed in 0.047706s
There was a problem hiding this comment.
That's interesting, I'm running linux so that's why I didn't catch it, but thanks for bringing that up. You can push the code so I can take a look and run the CI if everything looks alright.
szymmis
changed the title
IndexMiddleware and ViteIndexMiddleware currently do not take the base into account and always mount at "/".
Providing developers with the option to decide between mounting both index middlewares at the root or a specified base would enhance flexibility in configuration.