lens: fixes BASE_PATH handling and re-aligns to old resource paths #3722
Conversation
The last commit moved things forward, but it broke how we used the BASE_PATH variable. This reverts to the old logic and fixes up the build so it behaved like the last one. Along the way, I discovered some tests were disabled, and as able to fix them except one that I need @anuraaga or similar's help on. @SamTV12345 as a matter of some urgency, once this PR is merged, can you follow-up and test i18n? I noticed the language toggle doesn't work, as it just is always english. Signed-off-by: Adrian Cole <[email protected]>
|
|
||
| /* eslint-disable global-require */ | ||
|
|
||
| import { afterEach, it, describe, expect, vi } from 'vitest'; |
There was a problem hiding this comment.
this file was deleted in the last commit. I resurrected it.
| expect(screen.getAllByText('Suspended')).length(1); | ||
|
|
||
| fetchMock.called(UI_CONFIG); | ||
| }); | ||
|
|
||
| it('provides config when resolved', async () => { | ||
| it('provides config when resolved', (context) => async () => { |
There was a problem hiding this comment.
since this file was renamed in the last commit, none of the tests ran. I was able to get the former to run and pass, but it is a bit hacky as I really have no idea what I'm doing.
@anuraaga this is on a shared branch.. I think your old test was trying to show that the UI can work while the config.json request hasn't completed yet? I tried for over an hour to try to get this to work, so am not bothering you randomly.. could really use a hand (even you can paste) if you can help get this working.
There was a problem hiding this comment.
#3724 for follow-up
| @@ -16,6 +16,7 @@ | |||
| <!DOCTYPE html> | |||
| <html> | |||
| <head> | |||
| <base href="/zipkin" > | |||
There was a problem hiding this comment.
without this, anything that emulates zipkin-server's ZIPKIN_UI_BASE_PATH handling would have broke.
| "version": "0.106.2", | ||
| "resolved": "https://registry.npmjs.org/three/-/three-0.106.2.tgz", | ||
| "integrity": "sha512-4Tlx43uoxnIaZFW2Bzkd1rXsatvVHEWAZJy8LuE+s6Q8c66ogNnhfq1bHiBKPAnXP230LD11H/ScIZc2LZMviA==" | ||
| "version": "0.161.0", |
There was a problem hiding this comment.
manually fixed the CVE again, as the last commit reverted the exception. That's completely unsurprising. Anyway, good news is no critical CVEs left (according to trivy)
| @@ -1,2 +0,0 @@ | |||
| BASE_PATH=/zipkin | |||
There was a problem hiding this comment.
none of these files are needed for npm run build, npm run test or npm run start, so removed all of them to avoid confusion of explaining why they are here.
|
|
||
| // Default create-react-app proxy only proxies AJAX requests by looking at Accept headers. We want | ||
| // to proxy any request though, mainly to support the Download JSON button. | ||
| const proxy = createProxyMiddleware(['**/api/**', '**/config.json'], { |
There was a problem hiding this comment.
this was ignored.. the config is now inside vite
| @@ -0,0 +1,2 @@ | |||
| # This is needed while dependencies rely on different versions of react | |||
| legacy-peer-deps=true | |||
There was a problem hiding this comment.
unless you put this here, you have to remember not just for npm install, but also audit
|
ps @tacigar @anuraaga the last release included the source map which is a lot bigger. I noticed we aren't including it at the moment, but maybe that's for the better, vs serving a 10MB sourcemap asset. It has a notable output in our (compressed) binary size. So, for example, slim went down from 41.9MB to 38.3MB due to not including the sourcemap. |
| }); | ||
|
|
||
| it('defaults to /zipkin with no base tag', async () => { | ||
| const { BASE_PATH } = await import('./api'); |
There was a problem hiding this comment.
ps I barely understand what I was doing here, but by behavior, I notice you have to defer anything you want reverted by resetModules. This was the only way I could monkey it to work, and if is not the correct way, please suggest an alternative!
|
merging as the build is broke otherwise. Appreciate any hand in follow-up on things noted! |
|
#3723 for follow-up on broken i18n |
The last commit moved things forward, but it broke how we used the BASE_PATH variable. This reverts to the old logic and fixes up the build so it behaved like the last one.
Along the way, I discovered some tests were disabled, and as able to fix them except one that I need @anuraaga or similar's help on.
@SamTV12345 as a matter of some urgency, once this PR is merged, can you follow-up and test i18n? I noticed the language toggle doesn't work, as it just is always english.