Fix broken Hot Reload of Plexus Package #2089

Wise-Wizard · 2024-01-05T03:46:03Z

Addresses Hot Reload Dysfunctionality in Plexus Package: Unresponsive to src Package Changes in UI

Implemented comprehensive modifications to rectify the malfunctioning Hot Reload feature in the Plexus package. This issue resulted in an inability to reflect changes made to the src package in the user interface (UI). The following refinements have been executed to mitigate the problem:

Conducted a meticulous analysis to identify and address the root cause of the Hot Reload dysfunction within the Plexus package.
Incorporated strategic adjustments package.json and individual imports as to point to the right directory instead of lib
Rigorously tested the implemented changes to validate their efficacy, with a particular focus on ensuring real-time reflection of src package modifications in the UI.

Testing Process:

The remedial changes underwent a robust testing process to ensure a comprehensive resolution to the Hot Reload issue:

Confirmed the restoration of expected Hot Reload functionality, facilitating the prompt reflection of changes made to the src package in the UI.

Checklist

To comply with the project's contribution guidelines, the following checklist items have been addressed:

✓ I have carefully read and followed the contributing guidelines.
✓ I have signed all commits.
✓ Unit tests for the new functionality have been added.
✓ Linting and testing steps have been successfully executed:
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Your constructive feedback is welcome and appreciated. Thank you for your time and consideration.

Signed-off-by: Wise-Wizard <[email protected]>

…d/jaeger-ui into Plexus_Bug/Hot_Reload

yurishkuro

thanks!

yurishkuro · 2024-01-05T05:45:41Z

Introduced targeted unit tests specifically designed to assess the Hot Reload functionality, all of which passed successfully.

was this something you meant to commit? I don't see any new test files.

Wise-Wizard · 2024-01-05T06:55:33Z

Introduced targeted unit tests specifically designed to assess the Hot Reload functionality, all of which passed successfully.

was this something you meant to commit? I don't see any new test files.

No, I apologize I was trying to keep the format same from my previous PR message and ended up including the Unit Test line as well. I have removed it now from the message!

Wise-Wizard · 2024-01-05T06:56:24Z

thanks!

No problem at all :)

Wise-Wizard · 2024-01-05T06:57:35Z

What to do about the date time unit test, it is totally unrelated from the changes I made and I guess it was persistent in previous PRs as well?

Wise-Wizard · 2024-01-05T12:34:05Z

Hi @yurishkuro , once this PR is merged, may I resume working on resolving the issue with the Graph Zoom Buttons

yurishkuro · 2024-01-05T16:52:32Z

I fixed the failing test #2091.

yurishkuro · 2024-01-05T16:55:55Z

packages/plexus/package.json

@@ -3,7 +3,7 @@
  "license": "Apache-2.0",
  "version": "0.2.0",
  "description": "Directed Graph React component",
-  "main": "lib/index.js",
+  "main": "src/index.tsx",


hold on, I don't think this is the right fix. When I talked about overrides I was referring to jaeger-ui package, similar to the other import fixes you did. But if you make this change in plexus then it breaks it as a standalone library, because once it's published (e.g. to npm) only the /lib folder is included in the bundle, not the sources (as far as I understand). Even if sources are included, it would not be usable from pure JS projects.

For example, Bard things there are multiple ways to do that: https://g.co/bard/share/12a255963a6f

Yes, you are right the src won't be included, even if it is included it won't be compatible because it is written in TypeScript. But, this was the only possible override which pointed all the imports to src and not lib. Changing it back to lib, I will have to individually modify all the import statements

no, don't change imports, try to look into override options

Got it will look into this

codecov · 2024-01-05T16:56:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (868bb05) 96.57% compared to head (a1073fd) 96.57%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2089   +/-   ##
=======================================
  Coverage   96.57%   96.57%           
=======================================
  Files         254      254           
  Lines        7620     7620           
  Branches     1986     1986           
=======================================
  Hits         7359     7359           
  Misses        261      261

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yurishkuro · 2024-01-05T17:03:10Z

packages/jaeger-ui/src/model/trace-dag/types/TDagPlexusVertex.tsx

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-import { TVertex } from '@jaegertracing/plexus/lib/types';
+import { TVertex } from '@jaegertracing/plexus/src/types';


I am actually somewhat skeptical about this - all the imports you fixed are referring to types. Types don't contain any functionality, so while useful when the types themselves are changing, they won't fix the actual hot reloading issue.

Also, see my other comment https://github.com/jaegertracing/jaeger-ui/pull/2089/files#r1443119010. I think if you find a way to instruct jaeger-ui package at a global level to prefer src instead of lib of Plexus, you may not need to change these imports.

Yes, but even if it is changed at global level. The individual import will specifically still point to the lib instead of src right? Correct me if I am wrong?

you have to look into those suggestions from Bard - my understanding is if you override at a global level then the build system will dynamically replace the imports

Understood!

Hi, @yurishkuro have been working on this since yesterday night. Tried various methods, and on modifying the tsconfig.json file, to point @jaegertracing/plexus to src package all the imports now automatically point to plexus/src now.

But, the problem that is not being resolved and I am stuck at is that Hot reload is still not working even after this.
Do you have any input that could be causing this?

It is really confusing, just manually changing the import statement to '@jaegertracing/plexus/src' is making the Hot Reload work completely fine, even though it points to the exactly same directory on hovering.

Hot reload is working perfectly fine in this.

yurishkuro · 2024-01-06T15:37:28Z

Tried various methods

please describe which override methods you tried

Wise-Wizard · 2024-01-06T15:48:30Z

Tried various methods

please describe which override methods you tried

Tried using alias in vite.config.mts file, and mostly spent time tinkering with tsconfig file of both plexus and jaeger-ui until the right baseUrl and paths were set so that @jaegertracing/plexus would point to the right path

yurishkuro · 2024-01-06T16:47:48Z

please show changes that you tried.

yurishkuro · 2024-01-06T17:01:45Z

I just read up more on this, and the two approaches that sound like they should work are

using Webpack rewriting (preferred, as it has mature support)
adding a custom import rewrite plugin to Vite

Wise-Wizard · 2024-01-06T17:18:30Z

Tried various methods

please describe which override methods you tried

This is the current override method which redirects the import to plexus/src

Wise-Wizard · 2024-01-06T17:28:37Z

I just read up more on this, and the two approaches that sound like they should work are

using Webpack rewriting (preferred, as it has mature support)

adding a custom import rewrite plugin to Vite

There is only a single webpack file, webpack-factory.js and in it the entry points are in src itself. Should I look into custom import rewrite to vite?

yurishkuro · 2024-01-06T19:49:50Z

This is the current override method which redirects the import to plexus/src

this is in packages/jaeger-ui/tsconfig.json, which doesn't sound like the right scope - we're talking about override at the build system level, not a TS compiler config which only affects some of the files in jaeger-ui.

yurishkuro · 2024-01-06T19:51:19Z

There is only a single webpack file, webpack-factory.js

That file is inside plexus. I am not suggesting to change plexus, but to change how jaeger-ui package imports it.

Wise-Wizard · 2024-01-07T04:13:33Z

This is the current override method which redirects the import to plexus/src

this is in packages/jaeger-ui/tsconfig.json, which doesn't sound like the right scope - we're talking about override at the build system level, not a TS compiler config which only affects some of the files in jaeger-ui.

Yes, but the import @jaegertracing/plexus were only present inside jaeger-ui.
So, I assumed that by changing the tsconfig inside jaeger-ui, it would automatically override all the import destinations, which it did

Wise-Wizard · 2024-01-07T04:14:46Z

There is only a single webpack file, webpack-factory.js

That file is inside plexus. I am not suggesting to change plexus, but to change how jaeger-ui package imports it.

Yes, that's what I am trying to say there are no global webpack files only the one inside plexus.
Hence, webpack rewriting is not possible at global level.
I checked for files at global level first itself

Wise-Wizard · 2024-01-07T04:32:50Z

Update: I tried changing the global tsconfig file as well in the same way to override the import. That also managed to make the import point to plexus/src, but the hot reload feature is still not working.

Wise-Wizard · 2024-01-07T04:58:37Z

I think I have figured out the issue, @jaegertraacong/plexus refers to the "Node Module" import not the specific file import. so, it is importing the whole module from npm that will automatically refer to the lib files and not src.

It has to do with npm library itself, there is no global override possible that would make it use the files in plexus, instead of the whole npm package itself

yurishkuro · 2024-01-07T05:29:17Z

there is no packages/jaeger-ui/package.json, how did you get it?
L105 in your screenshot says that the module is "resolved" to a local workspace project, not NPM

yurishkuro · 2024-01-07T05:34:21Z

There are examples of monorepos with vite.js in this thread:

Wise-Wizard · 2024-01-07T07:42:25Z

there is no packages/jaeger-ui/package.json, how did you get it?

L105 in your screenshot says that the module is "resolved" to a local workspace project, not NPM

Yes, there is a package.json in the github repo.

I haven't had much experience with configuration and package files, so I apologize for any inconvenience caused by my lack of familiarity in this area. I truly appreciate your patience and guidance.

Wise-Wizard · 2024-01-07T09:00:54Z

Hi @yurishkuro , Update: Finally found out a way to configure it, updated dependencies in vite.config.mts using alias and now hot reload is working perfectly fine 😇

Signed-off-by: Wise-Wizard <[email protected]>

yurishkuro · 2024-01-07T17:26:37Z

packages/jaeger-ui/tsconfig.json

+    "baseUrl": "..",
+    // other options...
+    "paths": {
+      "@jaegertracing/plexus": ["plexus/src"]


Is this needed given the vite.config change? Doing override in tsconfig means it only affects typescript source files, but we have a mix of ts/js, so I was looking for a more global override.

Yes, it is needed apparently it is required for the TypeScript compiler to need to know about our aliases so that it can compile all of our imported files accordingly. I think for js there are no such requirements, hence it will work for js files as well.

tsc-lint is failing
Error: [tsc-lint ] packages/plexus/src/DirectedGraph/builtins/PureEdges.tsx(17,22): error TS6059: File '/home/runner/work/jaeger-ui/jaeger-ui/packages/plexus/src/DirectedGraph/builtins/EdgePath.tsx' is not under 'rootDir' '/home/runner/work/jaeger-ui/jaeger-ui/packages/jaeger-ui'. 'rootDir' is expected to contain all source files.

Is this needed given the vite.config change? Doing override in tsconfig means it only affects typescript source files, but we have a mix of ts/js, so I was looking for a more global override.

Just checked reverting the tsconfig to default, and hot reload is working fine. Had only made the changes because this website mentioned it https://dev.to/tilly/aliasing-in-vite-w-typescript-1lfo
Pushing the changes now.

Signed-off-by: Wise-Wizard <[email protected]>

packages/jaeger-ui/vite.config.mts

yurishkuro

Awesome! Please add a comment and this is good to merge.

Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Saransh Shankar <[email protected]>

yurishkuro · 2024-01-07T18:14:15Z

Thanks!

Wise-Wizard · 2024-01-07T18:14:59Z

Thanks!

No Problem! Thanks a lot for your patience and guidance! Can I resume working on the zoom buttons now?

yurishkuro · 2024-01-07T18:33:36Z

Can I resume working on the zoom buttons now?

why do you feel you need to ask?

Wise-Wizard · 2024-01-07T18:36:24Z

Can I resume working on the zoom buttons now?

why do you feel you need to ask?

No, just wanted to confirm if there was something else you would like me to look at before resuming the work on zoom button fix. 😅

yurishkuro · 2024-01-07T18:42:48Z

this one #2093

Wise-Wizard · 2024-01-08T07:40:53Z

this one #2093

Sure, will look into it next!
Also, I have a query regarding your organization's potential participation in LFX and GSOC '24 for the Spring term. Could you kindly provide information on this matter?

yurishkuro · 2024-01-08T16:31:09Z

See jaegertracing/jaeger#5084. No projects decided for the summer yet.

Wise-Wizard · 2024-01-08T18:08:10Z

See jaegertracing/jaeger#5084. No projects decided for the summer yet.

Thanks! Looking forward to it.

Fixed broken Hot Reload of Plexus Package

62a09e1

Signed-off-by: Wise-Wizard <[email protected]>

Wise-Wizard requested a review from a team as a code owner January 5, 2024 03:46

Wise-Wizard requested review from pavolloffay and removed request for a team January 5, 2024 03:46

Wise-Wizard and others added 3 commits January 5, 2024 09:17

Merge branch 'main' into Plexus_Bug/Hot_Reload

7a88510

Ran yarn prettier

4f090e4

Signed-off-by: Wise-Wizard <[email protected]>

Merge branch 'Plexus_Bug/Hot_Reload' of https://github.com/Wise-Wizar…

46e1db6

…d/jaeger-ui into Plexus_Bug/Hot_Reload

yurishkuro changed the title ~~Fixed broken Hot Reload of Plexus Package~~ Fix broken Hot Reload of Plexus Package Jan 5, 2024

yurishkuro previously approved these changes Jan 5, 2024

View reviewed changes

yurishkuro added the changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements label Jan 5, 2024

Merge branch 'main' into Plexus_Bug/Hot_Reload

906182c

yurishkuro reviewed Jan 5, 2024

View reviewed changes

Fixed Global Overrirde for Plexus

fc268d0

Signed-off-by: Wise-Wizard <[email protected]>

Wise-Wizard dismissed yurishkuro’s stale review via fc268d0 January 7, 2024 09:12

Wise-Wizard added 2 commits January 7, 2024 14:44

Ran Yarn Prettier

5804bf5

Signed-off-by: Wise-Wizard <[email protected]>

Reverted change in DAG.tsx

6e8d3df

Signed-off-by: Wise-Wizard <[email protected]>

yurishkuro reviewed Jan 7, 2024

View reviewed changes

Wise-Wizard added 2 commits January 7, 2024 23:19

Reverted change in tsconfig.json

1bb1e50

Signed-off-by: Wise-Wizard <[email protected]>

Reverted change in tsconfig

6bb843a

Signed-off-by: Wise-Wizard <[email protected]>

yurishkuro reviewed Jan 7, 2024

View reviewed changes

packages/jaeger-ui/vite.config.mts Show resolved Hide resolved

yurishkuro previously approved these changes Jan 7, 2024

View reviewed changes

Update packages/jaeger-ui/vite.config.mts

a1073fd

Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Saransh Shankar <[email protected]>

Wise-Wizard dismissed yurishkuro’s stale review via a1073fd January 7, 2024 18:05

yurishkuro approved these changes Jan 7, 2024

View reviewed changes

yurishkuro merged commit 9fadfff into jaegertracing:main Jan 7, 2024
10 checks passed

Fix broken Hot Reload of Plexus Package #2089

Fix broken Hot Reload of Plexus Package #2089

Conversation

Wise-Wizard commented Jan 5, 2024 • edited Loading

Checklist

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Jan 5, 2024

Wise-Wizard commented Jan 5, 2024

Wise-Wizard commented Jan 5, 2024

Wise-Wizard commented Jan 5, 2024

Wise-Wizard commented Jan 5, 2024

yurishkuro commented Jan 5, 2024

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

codecov bot commented Jan 5, 2024 • edited Loading

Codecov Report

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

Wise-Wizard Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Jan 6, 2024

Wise-Wizard commented Jan 6, 2024 • edited Loading

yurishkuro commented Jan 6, 2024

yurishkuro commented Jan 6, 2024

Wise-Wizard commented Jan 6, 2024

Wise-Wizard commented Jan 6, 2024

yurishkuro commented Jan 6, 2024

yurishkuro commented Jan 6, 2024

Wise-Wizard commented Jan 7, 2024

Wise-Wizard commented Jan 7, 2024

Wise-Wizard commented Jan 7, 2024

Wise-Wizard commented Jan 7, 2024 • edited Loading

yurishkuro commented Jan 7, 2024

yurishkuro commented Jan 7, 2024

Wise-Wizard commented Jan 7, 2024 • edited Loading

Wise-Wizard commented Jan 7, 2024 • edited Loading

Choose a reason for hiding this comment

Wise-Wizard Jan 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Jan 7, 2024

Wise-Wizard commented Jan 7, 2024

yurishkuro commented Jan 7, 2024

Wise-Wizard commented Jan 7, 2024

yurishkuro commented Jan 7, 2024

Wise-Wizard commented Jan 8, 2024

yurishkuro commented Jan 8, 2024

Wise-Wizard commented Jan 8, 2024

Wise-Wizard commented Jan 5, 2024 •

edited

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading

Wise-Wizard Jan 6, 2024 •

edited

Loading

Wise-Wizard commented Jan 6, 2024 •

edited

Loading

Wise-Wizard commented Jan 7, 2024 •

edited

Loading

Wise-Wizard commented Jan 7, 2024 •

edited

Loading

Wise-Wizard commented Jan 7, 2024 •

edited

Loading

Wise-Wizard Jan 7, 2024 •

edited

Loading