QwikDev
fix(qwik-city): support vite.base in dev mode
Deploy request for qwik-insights pending review.
Visit the deploys page to approve it
| Name | Link |
|---|---|
| Latest commit | 646ba54f80e56126e868a40126295f8578d70712 |
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
@wmertens didn't finish yet. I create PR just for convenient sync code. lol
@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.
/build is where the scripts are stored by default. The result of this function should then get the vite base added.
Great, but we need some unit tests and more consistent
/handling
I'll add test after you reviewed.
@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
@JerryWu1234 which one specifically? I don't have any more information to add right now?
@JerryWu1234 which one specifically? I don't have any more information to add right now?
https://github.com/BuilderIO/qwik/pull/6017#discussion_r1533292949
https://github.com/BuilderIO/qwik/pull/6017#discussion_r1533286154
Here
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
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 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.
@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 🤔 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.
I think I can dynamically pass in the base parameter in both build and start modes
sounds good, give it a shot!
@JerryWu1234 there's a TS error https://github.com/BuilderIO/qwik/actions/runs/8508365123/job/23414373840?pr=6017#step:7:14
@JerryWu1234 there's a TS error https://github.com/BuilderIO/qwik/actions/runs/8508365123/job/23414373840?pr=6017#step:7:14
I got it. I'm write the test right now, and will fix it late
@wmertens https://github.com/BuilderIO/qwik/blob/646ba54f80e56126e868a40126295f8578d70712/packages/qwik/src/vite-basetest.unit.ts#L12 the function starts a qwik server and simulates passing in qwikCity() and qwikVite(). I referred to the dev-server code.
- Could you please evaluate the feasibility of my test plan?
- I can't write a complete test yet because there is an error. The error is a bit complicated. I hope you can help me take a look.
I propose you leave the tests out, on the v2 branch it will be clearer how to test e2e
https://github.com/QwikDev/qwik/blob/build/v2/starters/dev-server.ts
No different in both branches (master and build/v2) I have checked.
Could you please help me to point out where I add my test ?
@JerryWu1234 actually the e2e tests don't test qwik-city yet in v2, so we'll have to wait for that to add a test.
In the meantime we can merge this in v1
actually the e2e tests don't test qwik-city yet in v2, so we'll have to wait for that to add a test
ok . add tests late
last check before merge
dev
has /newbase/
no /newbase/
pro
has /newbase/
no /newbase/
I do need this but I do set base to a cloudfront url so the regexp may not work
I do need this but I do set
baseto a cloudfront url so the regexp may not work
good idea I will cover this function
@wmertens now is ok, I will do rest of the part after build/2‘test is improved