-
Notifications
You must be signed in to change notification settings - Fork 530
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.
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
Feat/migrate page ,index and TraceIdSearch to functional component #2673
base: main
Are you sure you want to change the base?
Feat/migrate page ,index and TraceIdSearch to functional component #2673
Conversation
Signed-off-by: Mubashirshariq <[email protected]>
692e8e7
to
e834cce
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2673 +/- ##
==========================================
- Coverage 96.59% 96.59% -0.01%
==========================================
Files 256 256
Lines 7727 7718 -9
Branches 2002 2024 +22
==========================================
- Hits 7464 7455 -9
Misses 263 263 ☔ View full report in Codecov by Sentry. |
componentDidMount() { | ||
const { pathname, search } = this.props; | ||
export const PageImpl: React.FC<TProps> = ({ children, embedded, pathname, search }) => { | ||
useEffect(() => { | ||
trackPageView(pathname, search); |
There was a problem hiding this comment.
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) {
There was a problem hiding this comment.
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.
} | ||
// Initialize API and process scripts | ||
JaegerAPI.apiRoot = DEFAULT_API_ROOT; | ||
processScripts(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Signed-off-by: Mubashirshariq <[email protected]>
useEffect(() => { | ||
JaegerAPI.apiRoot = DEFAULT_API_ROOT; | ||
processScripts(); | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useEffect
runs after the component mounts initially, while constructors are executed before the component mounts.
A better alternative would be to use a custom hook that uses a ref/state to keep track of the functional. Read a nice article here that explains why functional components lack this constructor
alternative and what should be the workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but i think here it needs to be inside the useEffect
since processScripts() directly interacts with DOM,so it should be rendered every time the component is mounted
Which problem is this PR solving?
Migrating Class components to functional components
/attempt #2610
Description of the changes
migrate page,index and traceidsearch
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:npm run lint
andnpm run test