Skip to content
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

Jump to bottom

refactor(cdk:utils): add some methods to control subscribers in createSharedComposable #1519

Closed
wants to merge 3 commits into from
Closed

refactor(cdk:utils): add some methods to control subscribers in createSharedComposable #1519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
935b176
fix(comp:modal): destroyOnHide cause can't drag
kovsu Mar 27, 2023
2c8d9b5
fix(cdk:drag-drop): root lose watch but still use
kovsu Mar 28, 2023
74aba42
refactor(cdk:utils): subscriber manipulation
kovsu Mar 31, 2023
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
2 changes: 1 addition & 1 deletion packages/cdk/a11y/src/focusMonitor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link

Choose a reason for hiding this comment

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

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.

4 changes: 3 additions & 1 deletion packages/cdk/a11y/src/inputModalityDetector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,4 +223,6 @@ export function useInputModalityDetector(options?: InputModalityDetectorOptions)
}
}

export const useSharedInputModalityDetector = createSharedComposable(() => useInputModalityDetector())
export const { useComposable: useSharedInputModalityDetector } = createSharedComposable(() =>
useInputModalityDetector(),
)
Copy link

Choose a reason for hiding this comment

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

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.

2 changes: 1 addition & 1 deletion packages/cdk/breakpoint/src/breakpoints.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link

Choose a reason for hiding this comment

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

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.

9 changes: 7 additions & 2 deletions packages/cdk/drag-drop/src/composables/useDraggable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
* found in the LICENSE file at https://github.com/IDuxFE/idux/blob/main/LICENSE
*/

import type { DnDContext } from './useDragDropContext'
import type { DnDEventName, DnDEventType, DnDPosition, DraggableOptions } from '../types'

import { type ComputedRef, computed, ref, watch } from 'vue'

import { type MaybeElementRef, convertElement, tryOnScopeDispose, useEventListener } from '@idux/cdk/utils'

import { DnDContext } from './useDragDropContext'
import { withBoundary } from './withBoundary'
import { withDragFree } from './withDragFree'
import { withDragHandle } from './withDragHandle'
Expand All @@ -37,7 +37,8 @@ export function useDraggable(
reset: () => void
stop: () => void
} {
context = initContext(context)
const { context: _context, delSubscribers } = initContext(context)
context = _context

const registry = context.registry!
const stateRef = ref<DnDState>()
Expand Down Expand Up @@ -68,6 +69,10 @@ export function useDraggable(
}

const offDraggable = (sourceElement: HTMLElement) => {
if (delSubscribers) {
delSubscribers()
}

registry.off(sourceElement)
listenerStops.forEach(listenerStop => listenerStop())

Copy link

Choose a reason for hiding this comment

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

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.

Expand Down
18 changes: 14 additions & 4 deletions packages/cdk/drag-drop/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,27 +11,37 @@ import { createSharedComposable } from '@idux/cdk/utils'

import { type DnDContext, _dnDContextMap, useDragDropContext } from './composables/useDragDropContext'

const useDnDContext = createSharedComposable(useDragDropContext)
const { useComposable: useDnDContext, addSubscribers, delSubscribers } = createSharedComposable(useDragDropContext)

/**
* get context
*
* @param targetContext
*/
export const initContext = (targetContext?: DnDContext): DnDContext => {
export const initContext = (
targetContext?: DnDContext,
): {
context: DnDContext
delSubscribers?: () => void
} => {
let context = {} as DnDContext
// default context is window
if (!targetContext) {
const root = document as unknown as HTMLElement

if (!_dnDContextMap.has(root)) {
context = useDnDContext(root)
_dnDContextMap.set(root, context)
return { context }
} else {
addSubscribers()
context = _dnDContextMap.get(root)!
return { context, delSubscribers }
}
return context
}
return targetContext
return {
context: targetContext,
}
}

/**
Copy link

Choose a reason for hiding this comment

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

the code review

  1. The changes made in the code have been done efficiently, as a better alternative to the existing code.
  2. The use of 'createSharedComposable' method, makes the code more efficient.
  3. 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.
  4. The code is well-structured, making it easier to understand and read.

Expand Down
21 changes: 18 additions & 3 deletions packages/cdk/utils/src/composable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,27 +13,42 @@ import { callEmit } from './props'
import { tryOnScopeDispose } from './tryOnScopeDispose'

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export function createSharedComposable<T extends (...args: any[]) => ReturnType<T>>(composable: T): T {
export function createSharedComposable<T extends (...args: any[]) => ReturnType<T>>(
composable: T,
): { useComposable: T; addSubscribers: () => void; delSubscribers: () => void } {
let subscribers = 0
let state: ReturnType<T> | undefined
let scope: EffectScope | undefined

const dispose = () => {
if (scope && --subscribers <= 0) {
subscribers -= 1

if (scope && subscribers <= 0) {
scope.stop()
state = scope = undefined
}
}

return ((...args) => {
const addSubscribers = () => {
subscribers++
}

const delSubscribers = () => {
dispose()
}

const useComposable = ((...args) => {
subscribers++

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] {
Copy link

Choose a reason for hiding this comment

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

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.

Expand Down
3 changes: 1 addition & 2 deletions packages/components/modal/src/ModalProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,9 @@ export default defineComponent({
// The default value for `visible`, onDestroy should not be passed in Modal
const { key, visible = true, destroyOnHide, onDestroy, content, contentProps, ...restProps } = item
const setRef = (instance: unknown) => setModalRef(key!, instance as ModalInstance | null)
const onUpdateVisible = (visible: boolean) => update(key!, { visible })
const onAfterClose = destroyOnHide ? () => destroy(key!) : undefined
const onUpdateVisible = (visible: boolean) => update(key!, { visible })
const mergedProps = { key, visible, ref: setRef, 'onUpdate:visible': onUpdateVisible, onAfterClose }

const contentNode = isVNode(content) ? cloneVNode(content, contentProps, true) : content
return <Modal {...mergedProps} {...restProps} __content_node={contentNode} />
})
kovsu marked this conversation as resolved.
Show resolved Hide resolved
kovsu marked this conversation as resolved.
Show resolved Hide resolved
kovsu marked this conversation as resolved.
Show resolved Hide resolved
Expand Down