Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: add bridgeHook plugin system to support lifecycyle in bridge #2992
base: main
Are you sure you want to change the base?
feat: add bridgeHook plugin system to support lifecycyle in bridge #2992
Changes from 38 commits
3118053
3082116
f716f18
edaffa2
62480b6
c868988
cf83897
aa05fc5
8aebc66
e414ded
f575ec7
fe9ad08
7257784
74f057a
14a30fa
b47ea80
b02d9f0
93627d9
5e33da2
83e54ad
2fac2ea
4e319bb
a891716
5c7f0be
a6c9c97
10bc78b
19acf8b
25d850b
8588609
de94715
fc7b133
f673f69
4431f2e
324fcbc
93f3103
d779f32
02ff2b2
cd40591
8ff7ef6
dfb6d77
f2e9624
b25a91b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The BridgeHooks interface could benefit from standardizing the parameters. Consider creating a base params type and extending it:
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.
The lifecycle hook handling could be simplified and made more type-safe. Consider extracting the beforeBridgeRender logic into a separate function and adding type guards:
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.
Similar to the beforeBridgeRender hook, the afterBridgeDestroy hook handling could be extracted and simplified. The null checks could be combined:
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.
The className concatenation could be improved to handle undefined className prop. Also, consider using a more semantic HTML element:
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.
The router configuration lacks TypeScript type definitions which could lead to runtime errors. Consider adding proper type annotations for the router configuration object.
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.
The
createBrowserRouter
configuration should include error handling for missing or undefined properties. Here's a safer implementation:This ensures the router won't throw errors if properties are undefined and provides sensible defaults.
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.
The BridgeHooks type could be more strongly typed by including the expected return type for beforeBridgeRender. Since it can return extraProps, consider:
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.
There's a bug in the afterBridgeRender hook assignment. It's incorrectly using afterBridgeDestroy from bridgeInfo.hooks:
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.
The destroy method should clean up the rootMap entry after unmounting to prevent memory leaks: