fix(qwik-city): support vite.base in dev mode

[JerryWu1234]

Fixes #5984

[netlify]

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	646ba54

[JerryWu1234]

@wmertens didn't finish yet.
I create PR just for convenient sync code. lol

[JerryWu1234]

@wmertens I'm sorry to bother you, I know where problem is, but I was stuck on a crucial problem.

please take a look at this line

Why does it set a default '/build/',
and what's between q:route and q:base.

[wmertens]

/build is where the scripts are stored by default. The result of this function should then get the vite base added.

[wmertens]

Btw, you can set minify:false in the build to see prod as-is

[JerryWu1234]

pro

dev

everything is right

[JerryWu1234]

I also tested env and pro without base added

[wmertens]

Great, but we need some unit tests and more consistent / handling

[JerryWu1234]

Great, but we need some unit tests and more consistent / handling

I'll add test after you reviewed.

[wmertens]

@JerryWu1234 how's it going with the tests? Need help?

[JerryWu1234]

@JerryWu1234 how's it going with the tests? Need help?

@wmertens I haven't tried to add tests yet because I’m waiting for your reply on the problem you mentioned

[wmertens]

@JerryWu1234 which one specifically? I don't have any more information to add right now?

[JerryWu1234]

@JerryWu1234 which one specifically? I don't have any more information to add right now?

#6017 (comment)

#6017 (comment)

Here

[wmertens]

So I meant that we should have unit tests, and that you shouldn't have to replace the double slashes if you don't put them in.

So you need to be careful about base, don't call replace, and have unit tests that prove it doesn't add double slashes for various test cases

[JerryWu1234]

So I meant that we should have unit tests, and that you shouldn't have to replace the double slashes if you don't put them in.

So you need to be careful about base, don't call replace, and have unit tests that prove it doesn't add double slashes for various test cases

So I meant that we should have unit tests, and that you shouldn't have to replace the double slashes if you don't put them in.

So you need to be careful about base, don't call replace, and have unit tests that prove it doesn't add double slashes for various test cases

Let me try to add some tests

[JerryWu1234]

@JerryWu1234 how's it going with the tests? Need help?

@wmertens

I really need your help.

there is a little tricks, To test the base attribute, we need to rely on both build and start. However, the current e2e only has build.

If I modify it, I'm afraid it will break the project's testing structure.

[wmertens]

@JerryWu1234 🤔 actually the tests in v2 are a lot better. Take a look at the build/V2 branch, do you think you can incorporate your test there?

And actually, maybe that's not needed. It could be enough to make unit tests, not e2e

[JerryWu1234]

@JerryWu1234 🤔 actually the tests in v2 are a lot better. Take a look at the build/V2 branch, do you think you can incorporate your test there?

And actually, maybe that's not needed. It could be enough to make unit tests, not e2e

https://github.com/BuilderIO/qwik/blob/87f0b9e5acf89f03204cc8e1cddaca8f54ad8ee9/starters/playwright.config.ts#L28
https://github.com/BuilderIO/qwik/blob/87f0b9e5acf89f03204cc8e1cddaca8f54ad8ee9/starters/dev-server.ts#L63
The build/v2 branch does not seem to have many changes compared to the master branch. It also seems that it is not possible to dynamically pass in the base parameter.

Here is my idea:

I referenced the Astro test code, and I think I can dynamically pass in the base parameter in both build and start modes. Vitest can cover most scenarios, but I don't know how to test the qwikLoader function in Vitest, , so I want to use e2e tests.

[wmertens]

I think I can dynamically pass in the base parameter in both build and start modes

sounds good, give it a shot!

[PatrickJS]

yeah very weird

[PatrickJS]

oh maybe its erroring since the e2e tests are running the app in a browser so if the files can't load. thats the only thing i can think of

[PatrickJS]

idk about this I think we need to make sure /build/ is added for scripts even if base was set

[wmertens]

I think we need to take a step back here. q:base is the absolute path or full URL of where Qwik is supposed to get the dist files, and vite base is the prefix path to the url of the application.

• We need to make sure we use the right thing at the right time.
• q:base needs to be absolute or a full URL and end with /. It needs to include the vite base if it's not explicitly set.
• vite base needs to be absolute and end with /

I see a bunch of duplication around / handling, we can't have that. It needs to be simple throughout, with a helper function if needed. We know that the bases will always end with /.

Furthermore, it's up to the dev to use the vite base in their URLs, we can't add it for them, because there might be links between microfrontends.

[JerryWu1234]

I think we need to take a step back here. q:base is the absolute path or full URL of where Qwik is supposed to get the dist files, and vite base is the prefix path to the url of the application.

• We need to make sure we use the right thing at the right time.
• q:base needs to be absolute or a full URL and end with /. It needs to include the vite base if it's not explicitly set.
• vite base needs to be absolute and end with /

I see a bunch of duplication around / handling, we can't have that. It needs to be simple throughout, with a helper function if needed. We know that the bases will always end with /.

Furthermore, it's up to the dev to use the vite base in their URLs, we can't add it for them, because there might be links between microfrontends.

just want to confirm I understand or not.
such as if user set a Vite base: '/newbase/'
so that the q:base is http://localhost:3000/newbase/ in dev
and the q:base is http://localhost:3000/newbase/build/ in pro

@PatrickJS @wmertens

I think we can try to easy way to fix this bug and waiting for built/v2.
because it's very difficult to guarantee whether it's right or not without tests, and it's also tough to test code is right ot not

[PatrickJS]

q:base is for assets so it's saying this is where the /build/ files are. I think it makes sense right now to always make sure /build/ is in the url

[netlify]

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	9c23d05

[wmertens]

Actually I wonder if b381b22 fixed this - can you check if 1.5.5 still needs this PR @JerryWu1234 ?

[JerryWu1234]

Actually I wonder if b381b22 fixed this - can you check if 1.5.5 still needs this PR @JerryWu1234 ?

Let me check tomorrow

[JerryWu1234]

still has a problem

[wmertens]

Thank you for checking @JerryWu1234, can you figure out exactly what is wrong? Is the q:base wrong, or is it requesting specific files wrongly etc

[JerryWu1234]

Thank you for checking @JerryWu1234, can you figure out exactly what is wrong? Is the q:base wrong, or is it requesting specific files wrongly etc

Just check where wrong is? or fix it in this PR?

[wmertens]

@JerryWu1234 yes just where. Is it by any chance when it's trying to load /src/something..._hashhashhash.js? Then I know where to fix it (and I'm replacing that anyway)

[PatrickJS]

I'll fix the pr

[JuanmaVargas]

In production, is there any workaround to fix this? I'm trying to create a multi-zone in Next.js for a Qwik application, and for that, I need the vite.base set in the configuration to work for all the assets

[PatrickJS]

I'll revert my changes except for the important ones and update the pr

[pkg-pr-new]

Run & review this pull request in StackBlitz Codeflow.

@builder.io/qwik (9c23d05)

npm i https://pkg.pr.new/@builder.io/qwik@6017

@builder.io/qwik-city (9c23d05)

npm i https://pkg.pr.new/@builder.io/qwik-city@6017

eslint-plugin-qwik (9c23d05)

npm i https://pkg.pr.new/eslint-plugin-qwik@6017

create-qwik (9c23d05)

npm i https://pkg.pr.new/create-qwik@6017

[PatrickJS]

@JuanmaVargas can you use this to test out the changes

[wmertens]

@JerryWu1234 thank you for your efforts, but I think it's fixed now in 1.7.1 (in a different way) so I'll close this.