jaegertracing / jaeger-ui

Web UI for Jaeger
http://jaegertracing.io/
Apache License 2.0
1.15k stars 485 forks source link

Reimplement Monitor view as React functional component with hooks #2013

Open yurishkuro opened 11 months ago

yurishkuro commented 11 months ago

packages/jaeger-ui/src/components/Monitor/ServicesView/index.tsx is a class based component with pretty convoluted state management. It can be simplified if implemented as a functional component with hooks.

References:

Related/similar PRs:

takuabonn commented 11 months ago

hi @yurishkuro I want to work in this issue. will u assign it to me?

yurishkuro commented 11 months ago

Go ahead

Sweetdevil144 commented 11 months ago

So, we're gonna flip the MonitorATMServicesViewImpl class to a functional component. Here's the game plan of how I would proceed with this issue:

  1. State with useState: For each state variable in your class component, we'll switch to using useState. It's pretty straightforward. Like, if you have a state variable graphWidth in your class, it turns into:

    const [graphWidth, setGraphWidth] = useState(300);

    Repeat this for each piece of state (serviceOpsMetrics, searchOps, etc.).

  2. Lifecycle Methods with useEffect: Your componentDidMount, componentDidUpdate, and componentWillUnmount all get replaced by useEffect. Here's an example that covers component mounting and unmounting logic:

    useEffect(() => {
        fetchServices(); // Replace with your actual data fetching or setup code
        // Any additional setup code goes here
    
        return () => {
            // This is your cleanup code, like componentWillUnmount
            // e.g., remove event listeners
        };
    }, []); // The empty array makes it run only once on mount and unmount
  3. Refactor Methods to Functions: Convert class methods into functions. For instance, if you have a method for updating dimensions in your class, it would be something like:

    const updateDimensions = () => {
        // Your logic to update dimensions
    };
  4. Handling Redux: If you're using Redux, you'll use useSelector for selecting state and useDispatch for dispatching actions. Like this:

    const services = useSelector(state => state.services);
    const dispatch = useDispatch();
    
    // When you need to dispatch an action
    const fetchMetrics = () => {
        dispatch(fetchMetricsAction());
    };
  5. Testing: After refactoring, test to ensure all functionalities work as expected. Check if state updates correctly, effects run as intended, and the component behaves as it did before.

The goal is to keep the logic intact but just translate it into the functional component style. Hope this helps you envision how the refactored component will look! Let me know if there's a specific part you want more detail on.

EshaanAgg commented 7 months ago

Hi! Is this issue available to work on?

yurishkuro commented 7 months ago

@EshaanAgg yes, since there is no PR attached

RISHIKESHk07 commented 6 months ago

@yurishkuro how to check if my code is running correct after changing the component

RISHIKESHk07 commented 6 months ago

@yurishkuro i did rewrite the monitor , i don 't see any error except for the below , is there anything i missed ''' 7:56:27 PM [vite] http proxy error at /api/services: Error: connect ECONNREFUSED 127.0.0.1:16686 at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1595:16) (x2) ''' image

yurishkuro commented 6 months ago

why would making Reach changes result in any network-related errors?

Ghaby-X commented 3 months ago

@Sweetdevil144 I did rewrite the monitorServiceView as a functional component but, I could not pass all the test. The App component is being wrapped in the redux Provider; however it seems the test is not able to recognize the provider. Is there anything I missed?

image

This is a picture of the error

My current solution is at https://github.com/Ghaby-X/jaeger-ui/commit/1c133b31b4d730c3773196e45934e60964687b19

Srish-ty commented 4 weeks ago

@yurishkuro is this issue open to work on? asking as I see a merged pr already associated with this issue

yurishkuro commented 4 weeks ago

if it's open it's available. I don't see PRs.