Skip to content

Feat/migrate page ,index and TraceIdSearch to functional component #2673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Feat/migrate page ,index and TraceIdSearch to functional component #2673

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e834cce
migrate page index and traceidsearch to functional comp
Mubashirshariq Feb 18, 2025
87e41f9
added configuration steps inside effect
Mubashirshariq Feb 22, 2025
ec34877
Merge branch 'main' into fix/change-class-based-cops
yurishkuro Jul 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 19 additions & 29 deletions packages/jaeger-ui/src/components/App/Page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { Layout } from 'antd';
import cx from 'classnames';
import Helmet from 'react-helmet';
import { connect } from 'react-redux';
import { useEffect } from 'react';

import TopNav from './TopNav';
import { ReduxState } from '../../types';
Expand All @@ -36,38 +37,27 @@ type TProps = {
const { Header, Content } = Layout;

// export for tests
export class PageImpl extends React.Component<TProps> {
componentDidMount() {
const { pathname, search } = this.props;
export const PageImpl: React.FC<TProps> = ({ children, embedded, pathname, search }) => {
useEffect(() => {
trackPageView(pathname, search);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously this was called conditionally. What happens now?

if (pathname !== nextPathname || search !== nextSearch) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yurishkuro The conditional check if (pathname !== nextPathname || search !== nextSearch) was removed because useEffect's dependency array [pathname, search] handles this optimization automatically. The effect will only run when either dependency changes, making the manual comparison redundant. This achieves the same optimization with cleaner code.

}
}, [pathname, search]);

componentDidUpdate(prevProps: Readonly<TProps>) {
const { pathname, search } = prevProps;
const { pathname: nextPathname, search: nextSearch } = this.props;
if (pathname !== nextPathname || search !== nextSearch) {
trackPageView(nextPathname, nextSearch);
}
}
const contentCls = cx({ 'Page--content': true, 'Page--content--no-embedded': !embedded });

render() {
const { embedded } = this.props;
const contentCls = cx({ 'Page--content': true, 'Page--content--no-embedded': !embedded });
return (
<div>
<Helmet title="Jaeger UI" />
<Layout>
{!embedded && (
<Header className="Page--topNav">
<TopNav />
</Header>
)}
<Content className={contentCls}>{this.props.children}</Content>
</Layout>
</div>
);
}
}
return (
<div>
<Helmet title="Jaeger UI" />
<Layout>
{!embedded && (
<Header className="Page--topNav">
<TopNav />
</Header>
)}
<Content className={contentCls}>{children}</Content>
</Layout>
</div>
);
};

// export for tests
export function mapStateToProps(state: ReduxState) {
Expand Down
35 changes: 14 additions & 21 deletions packages/jaeger-ui/src/components/App/TraceIDSearchInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,33 +27,26 @@ type Props = {
history: History;
};

class TraceIDSearchInput extends React.PureComponent<Props> {
goToTrace = (event: React.FormEvent<HTMLFormElement>) => {
const TraceIDSearchInput: React.FC<Props> = ({ history }) => {
const goToTrace = (event: React.FormEvent<HTMLFormElement>) => {
event.preventDefault();
const target = event.target as any;
const value = target.elements.idInput.value;
if (value) {
this.props.history.push(getUrl(value));
history.push(getUrl(value));
}
};

render() {
return (
<Form
data-testid="TraceIDSearchInput--form"
layout="horizontal"
onSubmitCapture={this.goToTrace}
className="TraceIDSearchInput--form"
>
<Input
data-testid="idInput"
name="idInput"
placeholder="Lookup by Trace ID..."
prefix={<IoSearch />}
/>
</Form>
);
}
}
return (
<Form
data-testid="TraceIDSearchInput--form"
layout="horizontal"
onSubmitCapture={goToTrace}
className="TraceIDSearchInput--form"
>
<Input data-testid="idInput" name="idInput" placeholder="Lookup by Trace ID..." prefix={<IoSearch />} />
</Form>
);
};

export default withRouteProps(TraceIDSearchInput);
112 changes: 55 additions & 57 deletions packages/jaeger-ui/src/components/App/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import React, { Component } from 'react';
import React from 'react';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The useEffect hook is used on line 81, but it's missing from the React import. Please update the import statement to:

import React, { useEffect } from 'react';

This will resolve the undefined reference to useEffect in the component.

Suggested change
import React from 'react';
import React, { useEffect } from 'react';

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

import { Provider } from 'react-redux';
import { Route, Redirect, Switch, Router } from 'react-router-dom';

Expand Down Expand Up @@ -77,62 +77,60 @@ const jaegerTheme = {
},
};

export default class JaegerUIApp extends Component {
constructor(props) {
super(props);
JaegerAPI.apiRoot = DEFAULT_API_ROOT;
processScripts();
}
// Initialize API and process scripts
JaegerAPI.apiRoot = DEFAULT_API_ROOT;
processScripts();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this ok to call this function unconditionally on importing index.jsx? Isn't there some lifecycle thing in functional components when it can be called? Is there an official recommendation for functions like this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought this is just a configuration step,which we can call at module level,but the processScripts interacts with DOM so it need to be in useEffect.updated it with useEffect so that it will run whenver the component mounts


render() {
return (
<ConfigProvider theme={jaegerTheme}>
<Provider store={store}>
<HistoryProvider history={history}>
<Router history={history}>
<Page>
<Switch>
<Route path={searchPath}>
<SearchTracePage />
</Route>
<Route path={traceDiffPath}>
<TraceDiff />
</Route>
<Route path={tracePath}>
<TracePage />
</Route>
<Route path={dependenciesPath}>
<DependencyGraph />
</Route>
<Route path={deepDependenciesPath}>
<DeepDependencies />
</Route>
<Route path={qualityMetricsPath}>
<QualityMetrics />
</Route>
<Route path={monitorATMPath}>
<MonitorATMPage />
</Route>
const JaegerUIApp = () => {
return (
<ConfigProvider theme={jaegerTheme}>
<Provider store={store}>
<HistoryProvider history={history}>
<Router history={history}>
<Page>
<Switch>
<Route path={searchPath}>
<SearchTracePage />
</Route>
<Route path={traceDiffPath}>
<TraceDiff />
</Route>
<Route path={tracePath}>
<TracePage />
</Route>
<Route path={dependenciesPath}>
<DependencyGraph />
</Route>
<Route path={deepDependenciesPath}>
<DeepDependencies />
</Route>
<Route path={qualityMetricsPath}>
<QualityMetrics />
</Route>
<Route path={monitorATMPath}>
<MonitorATMPage />
</Route>

<Route exact path="/">
<Redirect to={searchPath} />
</Route>
<Route exact path={prefixUrl()}>
<Redirect to={searchPath} />
</Route>
<Route exact path={prefixUrl('/')}>
<Redirect to={searchPath} />
</Route>
<Route exact path="/">
<Redirect to={searchPath} />
</Route>
<Route exact path={prefixUrl()}>
<Redirect to={searchPath} />
</Route>
<Route exact path={prefixUrl('/')}>
<Redirect to={searchPath} />
</Route>

<Route>
<NotFound />
</Route>
</Switch>
</Page>
</Router>
</HistoryProvider>
</Provider>
</ConfigProvider>
);
}
}
<Route>
<NotFound />
</Route>
</Switch>
</Page>
</Router>
</HistoryProvider>
</Provider>
</ConfigProvider>
);
};

export default JaegerUIApp;
Loading