refactor(cdk:utils): add some methods to control subscribers in createSharedComposable #1519
Conversation
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
|
This preview will be available after the AzureCI is passed. |
kovsu
force-pushed
the
fix-destroy-hide-modal
branch
from
b48ca8d
to
935b176
Compare
Codecov Report
@@ Coverage Diff @@
## main #1519 +/- ##
==========================================
- Coverage 92.75% 92.74% -0.01%
==========================================
Files 331 332 +1
Lines 30801 30923 +122
Branches 3533 3557 +24
==========================================
+ Hits 28568 28679 +111
- Misses 2233 2244 +11
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@tuchg 看下， document 作为 context 的时候，被意外的 cleanup 了，导致所有的事件监听都失效了。 |
| @@ -568,4 +568,4 @@ export function useFocusMonitor(options?: FocusMonitorOptions): FocusMonitor { | |||
| return { monitor, stopMonitoring, focusVia, blurVia } | |||
| } | |||
|
|
|||
| export const useSharedFocusMonitor = createSharedComposable(() => useFocusMonitor()) | |||
| export const { useComposable: useSharedFocusMonitor } = createSharedComposable(() => useFocusMonitor()) | |||
There was a problem hiding this comment.
the code review:
The code patch looks good and there do not seem to be any bugs or risks. It looks like the author is trying to create a shared composable for the useFocusMonitor() function. The new syntax used for export is the latest standard and allows for better readability. Overall, it looks like a solid patch.
| export const useSharedInputModalityDetector = createSharedComposable(() => useInputModalityDetector()) | ||
| export const { useComposable: useSharedInputModalityDetector } = createSharedComposable(() => | ||
| useInputModalityDetector(), | ||
| ) |
There was a problem hiding this comment.
with code review
The patch above looks valid and should work as expected. To make sure it is error-free, I would suggest running a linter or static analysis tool to check for any potential issues. Additionally, it could be beneficial to add unit tests to ensure the code behaves as intended. Finally, if feasible, consider refactoring the code to make it more readable and maintainable.
| @@ -43,4 +43,4 @@ export function useBreakpoints<T extends string>(value?: Record<T, string>): Rec | |||
| return match as Record<T, boolean> | |||
| } | |||
|
|
|||
| export const useSharedBreakpoints = createSharedComposable(() => useBreakpoints()) | |||
| export const { useComposable: useSharedBreakpoints } = createSharedComposable(() => useBreakpoints()) | |||
There was a problem hiding this comment.
code review.
The code patch looks good. It is updating the function useSharedBreakpoints to make it a shared composable. The new code is using the createSharedComposable function, which will enable the use of the useBreakpoints function in multiple components. There are no bugs risks seen in this patch.
It can be improved by adding comments to the code, which will help explain the change and clarify why it was done. Additionally, adding error handling to the useBreakpoints function would be beneficial, to ensure that the code handles unexpected errors gracefully.
| if (delSubscribers) { | ||
| delSubscribers() | ||
| } | ||
|
|
||
| registry.off(sourceElement) | ||
| listenerStops.forEach(listenerStop => listenerStop()) | ||
|
|
There was a problem hiding this comment.
The code looks alright. The main issue I can see is that the context variable is not declared with a const or let keyword, which may lead to unexpected behaviour. Also, the code does not have any comments which would help explain what it does and how it works.
Other than that, the code looks to be well written and organized. There are no major bug risks that I can see.
As for improvement suggestions, I would suggest adding comments to explain the purpose of each function and/or block of code. This would make the code easier to understand and maintain. Additionally, you could also add some type checking to ensure that the variables being passed in are of the correct type.
| return targetContext | ||
| return { | ||
| context: targetContext, | ||
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
the code review
- The changes made in the code have been done efficiently, as a better alternative to the existing code.
- The use of 'createSharedComposable' method, makes the code more efficient.
- The 'initContext' method has been improved to include the 'delSubscribers' function and also to check if the root is already present in the '_dnDContextMap' before creating one. This is a good practice as it avoids unnecessary creation of root context.
- The code is well-structured, making it easier to understand and read.
| if (!state) { | ||
| scope = effectScope(true) | ||
| state = scope.run(() => composable(...args)) | ||
| } | ||
| tryOnScopeDispose(dispose) | ||
| return state | ||
| }) as T | ||
|
|
||
| return { useComposable, addSubscribers, delSubscribers } | ||
| } | ||
|
|
||
| export function useState<T>(defaultOrFactory: T | (() => T), shallow = true): [ComputedRef<T>, (value: T) => void] { |
There was a problem hiding this comment.
the code review:
First, the code appears to be well commented and formatted, which helps to make it easy to read. It also follows best practices, such as using descriptive variable names and disabling eslint for specific lines.
The code is creating a function that returns an object with 3 properties - useComposable, addSubscribers, and delSubscribers. The useComposable function is used to create a composable function, while the other two properties are used to add and delete subscribers from the composable. The code also uses tryOnScopeDispose to ensure that any changes to the composable are properly disposed of when there are no more subscribers.
The only potential bug risk I can see is if the subscribers count is not updated correctly, which could result in the scope being stopped prematurely. To avoid this, it would be a good idea to use a more reliable method of tracking the number of subscribers, such as an array. Additionally, it might be helpful to add some additional logging to make sure that the subscribers count is accurate.
kovsu
changed the title
PR Checklist
Please check if your PR fulfills the following requirements:
What is the current behavior?
fix #1436
What is the new behavior?
Other information