Added vite config and updated deploy to gh-pages script.

[SamTV12345]

I already tested the build locally and it works with your current configuration. So you don't need to change any gh-pages settings.

[anuraaga]

@SamTV12345 Doh, sorry I forgot I am able to push to your branch, I had meant to push to my fork and send a PR to this one. I found having the duplicate files in the same repo to be a bit confusing (gh-pages had a nice aspect of shielding that to some extent). I figured it could be worth to have vite do the copy, and confirmed it with built and dev server. How does it look? If not good, feel free to revert, sorry for accidentally pushing

https://github.com/openzipkin/zipkin-api/pull/102/commits/08f96d6130d29b126e710e238abbbdbee320aef0

[SamTV12345]

@anuraaga Great idea. I just updated the license information but everything else looks great.

[codefromthecrypt]

actually the github actions test is already configured to run the root npm commands. We shouldn't have a mix of build tools in the same repo. Can you raise a different PR to make the root project use the same infra you are using here? you can update /pom.xml too to make sure it is downloading the correct stuff (run ./mvnw test to verify)

[SamTV12345]

actually the github actions test is already configured to run the root npm commands. We shouldn't have a mix of build tools in the same repo. Can you raise a different PR to make the root project use the same infra you are using here? you can update /pom.xml too to make sure it is downloading the correct stuff (run ./mvnw test to verify)

Sure. I'll do this in around 6 hours.

[codefromthecrypt]

so basically in the other pr, update /package* for the switch off npm (also need to update here https://github.com/openzipkin/zipkin-api/blob/master/pom.xml#L181). Please don't remove maven or anything else, mainly we are just setting on one tool for npm-like stuff. Then, we can rebase this PR over that and the instructions won't be like.. use npm if in the root folder, or if in the docs, use pnpm.

Also, I think this project directory should be named swagger or openapi as that's what it is building. I don't think there's any proto or thrift integration for swagger anyway right?

[codefromthecrypt]

maybe the best name for this directory is 'site'? I think docs is ok, but doesn't hint that it is published and maybe that's the most important part about it.

[codefromthecrypt]

last and most important comment for the night.

remember, the only reason why we are adding machinery here is if it can fix the XSS. Please make a comment what part of this fixes the XSS. If the result of this doesn't fix the XSS we lost the problem and started doing something else entirely. We have to be problem focused, not infra focused, as more infra means more things that can break.

[codefromthecrypt]

here's the XSS thing https://github.com/openzipkin/zipkin-api/issues/98

I was under the impression we were making a custom build to fix that, so basically if it doesn't fix that I really don't want a custom build, as it would end up me maintaining this when people lose interest.

[SamTV12345]

here's the XSS thing #98

I was under the impression we were making a custom build to fix that, so basically if it doesn't fix that I really don't want a custom build, as it would end up me maintaining this when people lose interest.

It does fix XSS. I built the api and entered the query parameter as shown in the example. It doesn't show an alert window or a different swagger api yaml

[codefromthecrypt]

great! glad it fixes it. In the future, put the fix verification into the PR desc to remove fear uncertainty and doubt (FUD) like I had.

next: what's most uncomfortable is that instead of modifying the existing javascript build (/package.json which verifies the swagger), we created a new project copying some of the files and making a completely different build. What I was expecting was a modification to the original build, not a new directoty where root tests the swagger and another project with different tooling publishes it.

Can you look into flattening this? modify the existing build, so no two projects which gives a problem of how to test both individually and what to name the thing that publishes what the other one tests?

[SamTV12345]

great! glad it fixes it. In the future, put the fix verification into the PR desc to remove fear uncertainty and doubt (FUD) like I had.

next: what's most uncomfortable is that instead of modifying the existing javascript build (/package.json which verifies the swagger), we created a new project copying some of the files and making a completely different build. What I was expecting was a modification to the original build, not a new directoty where root tests the swagger and another project with different tooling publishes it.

Can you look into flattening this? modify the existing build, so no two projects which gives a problem of how to test both individually and what to name the thing that publishes what the other one tests?

Done. I changed the config of the frontend eirslett plugin and moved the tests to the site folder.

[codefromthecrypt]

ps there's a comment out-of-date in build-bin test. Whenever changing a build tool, use ag, grep or your favorite search tool to make sure all references including doc comments are not saying something out of date.

[codefromthecrypt]

the reason this simple affect (stop the XSS) is taking many hours over several weeks, is that many things are combined into the same change. This adds labor as a lot of things have to be adjusted, and the important parts are held captive to unimportant parts.

For example, as you know right now, I really am not keen on everything here as it is much more work than before, not just during review, but forever.

The change says "Added vite config and updated deploy to gh-pages script." The goal was to remove the XSS

The change here also does things that steal review time and could be far simpler done independently in different PRs Like this:

• change the current build to pnpm
• change the validation script to typescript
• use vite build for openapi instead of swagger (yes this updates the gh-pages script)
• change non-swagger api publication from script to vite

[codefromthecrypt]

@SamTV12345 last comment is something we can avoid next time. Small changes at the same time as something related are one thing, but this changes a lot of things at once. This has taken many hours of my time, and really I don't even like the result vs overwriting the html. I do like parts of it! If something like the vite part fails later and we have to revert, we have to also revert the good parts. In general, I use the term "holding code hostage" when there's a PR that has very independent easy to say yes things mixed in with difficult parts. While I will continue to work with you to get this landed, let's please not do this next time, as I keep missing family time over things like this.

[codefromthecrypt]

I'm going to do max one more review passes on this. If things aren't mergable I have to set a personal boundary and close the PR. I would then make the script to remove the XSS and add README instructions how to make sure it is disabled. As a project lead, I can't spend so much time on things that are not required to solve a problem. cc @reta @anuraaga @shakuzen as until someone else triple checks things, it is just me doing this, and I'm getting burnt out on it.

[codefromthecrypt]

unless this is added to pom.xml we won't know the build is broken until master tries to deploy

--- a/pom.xml
+++ b/pom.xml
@@ -217,6 +217,17 @@
               <arguments>run test</arguments>
             </configuration>
           </execution>
+          <execution>
+            <id>pnpm run build</id>
+            <goals>
+              <goal>pnpm</goal>
+            </goals>
+            <phase>package</phase>
+            <configuration>
+              <arguments>run build</arguments>
+            </configuration>
+          </execution>
         </executions>
       </plugin>

[codefromthecrypt]

Unless someone else can reproduce it, I think we already fixed #98 when we updated the swagger dist recently. I think I must have not refreshed my browser after #101

Unless someone can reproduce this (I have tried several browsers including mobile), let's step back and not keep moving forward with a custom UI.

https://zipkin.io/zipkin-api/?configUrl=data:text/html;base64,ewoidXJsIjoiaHR0cHM6Ly9leHViZXJhbnQtaWNlLnN1cmdlLnNoL3Rlc3QueWFtbCIKfQ==

changes below are welcome, but let's not dig a hole with making our own UI dist. If anything, we can setup a GH action to download and update our gh-pages content if it is really necessary, but meanwhile I will raise a PR to update docs.

• change the current build to pnpm
• change the validation script to typescript
• nothing more than this, please!