lens: migrates from react-scripts to vite #3506

SamTV12345 · 2023-02-04T21:33:57Z

This pr introduces vite a very fast bundling tool for react and other languages. It contains a modern API, live reloading, env variables and many more features.

Once this pr is merged I will add some of these features like dynamic imports.

jcchavezs · 2023-02-04T22:08:08Z

Would you please explain what is vite and how would this improve the UI? cc @tacigar

…

SamTV12345 · 2023-02-04T22:13:27Z

Sure. Vite is the intended way to run a modern react application. React scripts from Facebook is very bloated and contains more than 1000 packages in the standard configuration. It doesn't support lazy loading resources and takes forever to start in dev mode. Vite solves this issue with a modern API and a nearly instant start.

I just finished implementing language changing. You can start the project by starting the jar which runs at port 9411 and then start the dev mode of the frontend with npm run dev. The values are currently hard coded. I will change this next to be better configurable.

SamTV12345 · 2023-02-05T11:02:31Z

I am nearly done. The only failing test is UIConfig.test.jsx. What should the base url of the UIConfig.test.js file be?

SamTV12345 · 2023-02-05T21:59:37Z

Feel free to review this pr.

SamTV12345 · 2023-02-10T17:48:16Z

Could somebody please approve running workflows? I'd like to check if the pipeline is working. @jcchavezs @tacigar

SamTV12345 · 2023-02-10T23:13:20Z

Hopefully this is the last round of workflows. Could you please trigger them again? @jcchavezs @tacigar

SamTV12345 · 2023-02-11T07:13:25Z

The relevant checks are working. I have no clue why the mysql build should fail when I only change the UI or why the IT test takes so long @jcchavezs @tacigar ?

SamTV12345 · 2023-03-15T21:52:30Z

Any chance to get this merged with higher priority @jcchavezs @tacigar ?

jcchavezs · 2023-04-16T21:22:06Z

Please rebase

SamTV12345 · 2023-04-16T21:37:17Z

Done.

SamTV12345 · 2023-05-21T14:53:13Z

Is the pull request so far okay or are there changes necessary?

jcchavezs · 2023-05-23T08:04:38Z

zipkin-lens/.env.production

@@ -0,0 +1,2 @@
+VITE_UI_URL=/zipkin


I don't think it is a good idea to check in env specific files.

This actually sets the ui url for the frontend. I'm not sure what you mean with "check in env specific files".

jcchavezs · 2023-05-23T08:05:35Z

Do you mind checking @tacigar ? Pinging @wu-sheng as @skywalking also uses lens.

SamTV12345 · 2023-08-07T19:10:04Z

Any update @jcchavezs ?

codefromthecrypt · 2024-01-28T01:26:04Z

@SamTV12345 would this sort out the dead react-scripts problem? It seems like it would. If so, would you be inclined to rebase? #3713

SamTV12345 · 2024-01-28T09:24:04Z

@codefromthecrypt I rebased the master branch.

SamTV12345 · 2024-01-28T09:29:14Z

Seems like eslint is not configured yet correctly.

codefromthecrypt · 2024-01-28T09:32:43Z

Thanks @SamTV12345, can you run ./mvnw com.mycila:license-maven-plugin:format and add a commit or force-push a new one to pass the license check? until then, we can't reach the part that actually runs the UI tests. If you can't and granted edits, I can do it for you also.

SamTV12345 · 2024-01-28T10:06:58Z

Thanks @SamTV12345, can you run ./mvnw com.mycila:license-maven-plugin:format and add a commit or force-push a new one to pass the license check? until then, we can't reach the part that actually runs the UI tests. If you can't and granted edits, I can do it for you also.

I ran the command.

SamTV12345 · 2024-01-28T18:52:53Z

Thanks @SamTV12345, can you run ./mvnw com.mycila:license-maven-plugin:format and add a commit or force-push a new one to pass the license check? until then, we can't reach the part that actually runs the UI tests. If you can't and granted edits, I can do it for you also.

If you'd like to you can start testing and reviewing the commit.

codefromthecrypt

Thanks for the progress! I made some notes as I think we want to keep variable names the same, so that there is less change to developers. Also I think you may need to run npm run lint to fix that, possibly you'll have to run the ./mvnw com.mycila:license-maven-plugin:format after

In general, to know what the PR will pass or fail, run ./mvnw clean install -pl ./zipkin/zipkin-lens.. this may save you time from testing with github

codefromthecrypt · 2024-01-28T23:48:11Z

zipkin-lens/.env.test

@@ -0,0 +1,2 @@
+VITE_UI_URL=http://localhost:51204/zipkin


So, in our README, we have instructions to set API_BASE env when building, and this is used for npm start to use a different base path for the target. I wonder if this is what you are doing here? If so, maybe rename the variable to API_BASE? If this can take from the env variable, then maybe we don't need these files, and things work the same?

codefromthecrypt · 2024-01-28T23:49:04Z

zipkin-lens/package.json

    "react-redux": "^7.1.0",
    "react-router-dom": "^5.0.1",
-    "react-scripts": "5.0.1",


glad this is gone ;)

codefromthecrypt · 2024-01-28T23:49:24Z

zipkin-lens/pom.xml

@@ -116,7 +116,7 @@
              <goal>npm</goal>
            </goals>
            <configuration>
-              <arguments>install</arguments>
+              <arguments>install --legacy-peer-deps</arguments>


need a comment above why

codefromthecrypt · 2024-01-28T23:49:35Z

zipkin-lens/pom.xml

@@ -262,7 +262,7 @@
                <configuration>
                  <executable>npm</executable>
                  <arguments>
-                    <argument>install</argument>
+                    <argument>install  --legacy-peer-deps</argument>


npm introduced this in Version 7 to restore compability with the program. To cite stackoverflow:

Due to the large number of modules that haven't specifically added React v17 (or more recently, React 18) as a peerDependency, it's now commonplace to encounter the unable to resolve dependency tree error when running npm installs within a v17 React application.

So it is pretty much required to use this flag. It does not break installations. It just makes sure that installation goes as smooth as possible. We could switch to pnpm which is more modern than npm. But I wouldn't introduce this in this pr.

so maybe make an XML comment similarly? I'm not contesting it, just why should stay in the source tree.

e.g.



codefromthecrypt · 2024-01-28T23:53:00Z

zipkin-lens/src/components/DiscoverPage/LookbackMenu.test.jsx


    render(
      <LookbackMenu close={close} onChange={onChange} lookback={lookback} />,
    );

    // Click outside of the component.
    fireEvent.click(document);
-
-    // TODO: Should comment out this line.


I'm ok removing this note as it doesn't explain why it should be uncommented.. and possibly it isn't valid anymore anyway. confusing comments are worse than none

codefromthecrypt · 2024-01-28T23:54:30Z

zipkin-lens/src/index.css

@@ -11,5 +11,4 @@
 * or implied. See the License for the specific language governing permissions and limitations under
 * the License.
 */
-
-@import '~vizceral-react/dist/vizceral.css';
+/*@import '~vizceral-react/dist/vizceral.css';*/


why is this commented out?

codefromthecrypt · 2024-01-28T23:55:40Z

zipkin-lens/src/util/locale.test.ts

- * the License.
- */
-
-import { getLocale, setLocale } from './locale';


should we make something like this for i18s.test.ts?

Could be beneficial to do.

codefromthecrypt · 2024-01-28T23:57:04Z

zipkin-lens/vite.config.ts


-/* eslint-disable global-require */
-
-describe('BASE_PATH', () => {


so we used to have instructions for setting BASE_PATH.. I think it is now VITE_API_URL? Is it possible to restore BASE_PATH as this feature was requested by users and better to keep name and instructions the same if possible

codefromthecrypt · 2024-01-29T00:06:00Z

zipkin-lens/index.html

    <meta charset="utf-8" />
-    <link rel="icon" href="%PUBLIC_URL%/favicon.ico" />


I guess you are removing this because there is nothing in vite like PUBLIC_URL in react-scripts?

Yes exactly. The base path for vite needs to be set in the vite config ts. Then all paths are prepended with the correct url.

codefromthecrypt · 2024-01-29T00:07:35Z

zipkin-lens/src/components/App/App.tsx

-                              component={DependenciesPage}
-                            />
-                          )}
+                    <BrowserRouter basename={import.meta.env.VITE_UI_URL}>


lets keep the variable names the same, as it is less for people to understand, unless literally VITE_UI_URL is the required name.

Suggested change

<BrowserRouter basename={import.meta.env.VITE_UI_URL}>

<BrowserRouter basename={import.meta.env.BASE_PATH}>

codefromthecrypt · 2024-02-08T10:42:15Z

@SamTV12345 because you raised this PR from the branch named master, I won't be able to rebase it without closing this PR. Let us know if you have time to complete this, or if someone else should create a new PR

SamTV12345 · 2024-02-08T10:51:10Z

Hi @codefromthecrypt , sorry for the silence. I'm studying CS for masters degree. I have time from today till end of march. So it should be pretty likely to get this pull request merged till then. I'll continue working on it this evening.

codefromthecrypt · 2024-02-08T11:45:18Z

@SamTV12345 epic! thanks for the context and your help!

SamTV12345 · 2024-02-08T16:24:02Z

@codefromthecrypt The pipelines are passing. There is a regression in the tests but doesn't seem to be introduced by me:

SamTV12345 · 2024-02-08T16:49:05Z

A rebase solved that issue too. So all pipelines are green. If you need more help or other code changes just ask. I've got now a lot of time till the next semester starts.

codefromthecrypt

great progress and I think we are very close!

codefromthecrypt · 2024-02-09T00:54:20Z

zipkin-lens/.env.production

@@ -0,0 +1 @@
+BASE_PATH=/zipkin


So, I think there are two concepts going on

what is the base path of the UI (to override in case it isn't "/zipkin"

how do you get to the API server (formerly API_BASE which is a URI)

Can you separate these out, and then try to match up the README with whatever the outcome is?

I separated the two variables API_BASE and BASE_PATH. You are definetely right. It is best to separate the two.

So, the README says this:

By default, API requests are proxied to http://localhost:9411. You can change this target using
the API_BASE environment variable, e.g., API_BASE=http://tracing.company.com npm start.

So being user-empathetic, best to check the README works or change the readme. I think API_BASE should be defaulted to http://localhost:9411 and BASE_PATH should be a path (like /zipkin), not a URI like BASE_PATH in the .env.test file.

Can you think through this, and converge with the README or change it and update? If BASE_PATH must be a URI not a path, then, we need to note that in the release notes and change the variable to BASE_URI

The BASE_PATH variable can be anything. I changed it to correctly reflect the values described in the README.

codefromthecrypt · 2024-02-09T00:58:48Z

Loving that green build! fyi @tacigar I will merge sam's work when he finishes unless you scream!

codefromthecrypt · 2024-02-09T01:01:23Z

@SamTV12345 In case you are interested in a varied experience our JS/TS tracing library is in dire need of help. If no one helps get it back into shape, we'll have to kill the project. If you have some time to polish it up, I'm sure you'll make a lot of friends openzipkin/zipkin-js#536

SamTV12345 · 2024-02-09T11:04:51Z

@SamTV12345 In case you are interested in a varied experience our JS/TS tracing library is in dire need of help. If no one helps get it back into shape, we'll have to kill the project. If you have some time to polish it up, I'm sure you'll make a lot of friends openzipkin/zipkin-js#536

Thanks for the invite. Sure I'll take a look at the JavaScript repository. Maybe I can polish this up. I've been maintaining Etherpad for 1 1/2 years so shouldn't be a problem to find my way through the code :).

SamTV12345 · 2024-02-10T19:59:56Z

@codefromthecrypt Should I just fork the js repository, make the changes and create a pull request?

codefromthecrypt · 2024-02-11T00:41:17Z

@codefromthecrypt Should I just fork the js repository, make the changes and create a pull request?

for starters, yep! and please make one not off the "master" branch, rather a change-specific one as that makes it easier for us to contribute to the PR if needed. Cheers!

codefromthecrypt

Last comment I think. If there's nothing left in the README that is out-of-date, once addressed, let's merge!

codefromthecrypt · 2024-02-11T00:44:48Z

zipkin-lens/.env.test

@@ -0,0 +1,2 @@
+BASE_PATH=/zipkin
+API_BASE=http://localhost:51204/zipkin


is this .env.test file used anywhere? if not, maybe we can delete it to remove questions about it or possible port conflicts. if this actually used in tests, I guess there's no way to have it use a random port e.g. by specifying port zero?

It is used during the tests so the test Browserrouter knows where to find the routes etc. I don't really know what happens if I skip the API_BASE variable. If you ever start with E2E tests you can then simply start Zipkin on port 51204.

good enough for me!

SamTV12345 · 2024-02-11T11:31:22Z

@codefromthecrypt Should I just fork the js repository, make the changes and create a pull request?

for starters, yep! and please make one not off the "master" branch, rather a change-specific one as that makes it easier for us to contribute to the PR if needed. Cheers!

Thanks. I'll take a look at it and see what can be improved.

codefromthecrypt · 2024-02-11T12:03:09Z

Thanks for all the work and patience @SamTV12345!

SamTV12345 · 2024-02-11T12:04:10Z

You're welcome @codefromthecrypt . It's been great working with you on this pull request :).

SamTV12345 marked this pull request as ready for review February 5, 2023 10:55

jcchavezs requested a review from tacigar May 23, 2023 08:03

jcchavezs reviewed May 23, 2023

View reviewed changes

tacigar mentioned this pull request Dec 16, 2023

Update React Scripts, ESLint and testing packages #3659

Merged

codefromthecrypt mentioned this pull request Jan 28, 2024

move off react-scripts #3713

Closed

codefromthecrypt reviewed Jan 29, 2024

View reviewed changes

Added vite.

d1d1a3a

SamTV12345 added 5 commits February 8, 2024 17:36

Updated license information.

4f0fc00

Removed linting errors.

355a1a1

Fixed all tests.

ccc7fb1

Use env variables.

9086fd7

Fixed installation of frontend.

d9ca264

codefromthecrypt reviewed Feb 9, 2024

View reviewed changes

SamTV12345 added 2 commits February 9, 2024 19:47

Fixed API_BASE and BASE_PATH

0f48aea

Adapted env vars to README

cb1e2b4

codefromthecrypt approved these changes Feb 11, 2024

View reviewed changes

codefromthecrypt merged commit 2ada407 into openzipkin:master Feb 11, 2024
12 checks passed

codefromthecrypt changed the title ~~Added vite.~~ lens: migrates from react-scripts to vite Feb 12, 2024


		/* eslint-disable global-require */

		describe('BASE_PATH', () => {

		<meta charset="utf-8" />
		<link rel="icon" href="%PUBLIC_URL%/favicon.ico" />

	<BrowserRouter basename={import.meta.env.VITE_UI_URL}>
	<BrowserRouter basename={import.meta.env.BASE_PATH}>

		@@ -0,0 +1,2 @@
		BASE_PATH=/zipkin
		API_BASE=http://localhost:51204/zipkin

lens: migrates from react-scripts to vite #3506

lens: migrates from react-scripts to vite #3506

Conversation

SamTV12345 commented Feb 4, 2023 • edited Loading

jcchavezs commented Feb 4, 2023 via email

SamTV12345 commented Feb 4, 2023

SamTV12345 commented Feb 5, 2023

SamTV12345 commented Feb 5, 2023

SamTV12345 commented Feb 10, 2023

SamTV12345 commented Feb 10, 2023

SamTV12345 commented Feb 11, 2023 • edited Loading

SamTV12345 commented Mar 15, 2023

jcchavezs commented Apr 16, 2023

SamTV12345 commented Apr 16, 2023

SamTV12345 commented May 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs commented May 23, 2023

SamTV12345 commented Aug 7, 2023

codefromthecrypt commented Jan 28, 2024

SamTV12345 commented Jan 28, 2024

SamTV12345 commented Jan 28, 2024

codefromthecrypt commented Jan 28, 2024

SamTV12345 commented Jan 28, 2024

SamTV12345 commented Jan 28, 2024

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Feb 8, 2024

SamTV12345 commented Feb 8, 2024

codefromthecrypt commented Feb 8, 2024

SamTV12345 commented Feb 8, 2024

SamTV12345 commented Feb 8, 2024

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Feb 9, 2024

codefromthecrypt commented Feb 9, 2024

SamTV12345 commented Feb 9, 2024

SamTV12345 commented Feb 10, 2024

codefromthecrypt commented Feb 11, 2024

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamTV12345 commented Feb 11, 2024

codefromthecrypt commented Feb 11, 2024

SamTV12345 commented Feb 11, 2024

SamTV12345 commented Feb 4, 2023 •

edited

Loading

SamTV12345 commented Feb 11, 2023 •

edited

Loading