From 7d6124494dca92c4f7da57512bc48039d90282a5 Mon Sep 17 00:00:00 2001 From: productdevbook Date: Sun, 1 Dec 2024 10:03:56 +0300 Subject: [PATCH 1/9] refactor: enhance performance tracking and update useControllableStateV6handling in useControl fix: reactive refactor: enhance performance tracking and update handling in useControllableStateV6 refactor: optimize performance tracking and value handling in useControllableStateV6 refactor: optimize cloning and equality checks in useControllableStateV6 refactor: improve performance tracking and equality checks in useControllableStateV6 refactor: update performance results and enhance type handling in useControllableStateV6 refactor: update performance results and optimize state handling in useControllableStateV6 --- packages/core/.gitignore | 1 + packages/core/src/hooks/index.ts | 2 +- .../src/hooks/tests/performance-results.ts | 109 +++++++ .../hooks/tests/useControllableState.test.ts | 303 ++++++++++++++++++ .../core/src/hooks/useControllableState.ts | 137 +++++++- packages/core/src/slider/SliderRoot.ts | 17 +- .../core/src/slider/stories/SmallSteps.vue | 4 +- packages/performance-results.json | 29 ++ 8 files changed, 590 insertions(+), 12 deletions(-) create mode 100644 packages/core/src/hooks/tests/performance-results.ts create mode 100644 packages/core/src/hooks/tests/useControllableState.test.ts create mode 100644 packages/performance-results.json diff --git a/packages/core/.gitignore b/packages/core/.gitignore index 3b3246f37..8ad341737 100644 --- a/packages/core/.gitignore +++ b/packages/core/.gitignore @@ -32,3 +32,4 @@ storybook-static *.tsbuildinfo *storybook.log +.data \ No newline at end of file diff --git a/packages/core/src/hooks/index.ts b/packages/core/src/hooks/index.ts index cb835d695..30a08a9e1 100644 --- a/packages/core/src/hooks/index.ts +++ b/packages/core/src/hooks/index.ts @@ -1,7 +1,7 @@ export { createContext } from './createContext.ts' export { useBodyScrollLock } from './useBodyScrollLock.ts' export { useComposedElements } from './useComposedElements.ts' -export { useControllableState, useControllableStateV2, useControllableStateV3 } from './useControllableState.ts' +export { useControllableState, useControllableStateV2, useControllableStateV3, useControllableStateV5, useControllableStateV6 } from './useControllableState.ts' export { useEscapeKeydown } from './useEscapeKeydown.ts' export { useForwardElement } from './useForwardElement.ts' export { useId } from './useId.ts' diff --git a/packages/core/src/hooks/tests/performance-results.ts b/packages/core/src/hooks/tests/performance-results.ts new file mode 100644 index 000000000..b22c6eae5 --- /dev/null +++ b/packages/core/src/hooks/tests/performance-results.ts @@ -0,0 +1,109 @@ +import { existsSync, mkdirSync, writeFileSync } from 'node:fs' +import { resolve } from 'node:path' + +interface PerformanceResult { + version: string + updateCount: number + executionTimeMs: number + onChangeCallCount: number + timestamp: string +} + +export class PerformanceLogger { + private static results: PerformanceResult[] = [] + private static readonly outputPath = resolve(__dirname, './.data/performance-results.json') + + static logResult(version: string, executionTimeMs: number, onChangeCallCount: number, updateCount = 5) { + const result = { + version, + updateCount, + executionTimeMs, + onChangeCallCount, + timestamp: new Date().toISOString(), + } + + this.results.push(result) + this.writeToFile() // Write to file after each new result + return result + } + + static getResults() { + return [...this.results] + } + + static compareVersions() { + const v3Results = this.results.filter(r => r.version === 'useControllableStateV3') + const v5Results = this.results.filter(r => r.version === 'useControllableStateV5') + const v6Results = this.results.filter(r => r.version === 'useControllableStateV6') + const vueUseResults = this.results.filter(r => r.version === 'useVModel_VueUse') + + const v3Avg = this.calculateAverage(v3Results) + const v5Avg = this.calculateAverage(v5Results) + const v6Avg = this.calculateAverage(v6Results) + const vueUseAvg = this.calculateAverage(vueUseResults) + + const comparison = { + v3Average: v3Avg, + v5Average: v5Avg, + v6Average: v6Avg, + vueUseAverage: vueUseAvg, + performance: [] as string[], + } + + if (v3Avg && v5Avg && vueUseAvg && v6Avg) { + const implementations = [ + { name: 'V3', time: v3Avg.executionTimeMs }, + { name: 'V5', time: v5Avg.executionTimeMs }, + { name: 'V6', time: v6Avg.executionTimeMs }, + { name: 'VueUse', time: vueUseAvg.executionTimeMs }, + ].sort((a, b) => a.time - b.time) + + const fastest = implementations[0] + + implementations.slice(1).forEach((impl) => { + const diff = impl.time - fastest.time + const percentage = (diff / fastest.time) * 100 + comparison.performance.push( + `${fastest.name} is ${percentage.toFixed(2)}% faster than ${impl.name}`, + ) + }) + } + + return comparison + } + + private static calculateAverage(results: PerformanceResult[]) { + if (results.length === 0) + return null + + return { + executionTimeMs: results.reduce((sum, r) => sum + r.executionTimeMs, 0) / results.length, + onChangeCallCount: results.reduce((sum, r) => sum + r.onChangeCallCount, 0) / results.length, + } + } + + private static writeToFile() { + const output = { + timestamp: new Date().toISOString(), + results: this.results, + comparison: this.compareVersions(), + summary: this.compareVersions().performance, + } + + // .data directory must exist before writing to it + if (!existsSync(resolve(__dirname, './.data'))) { + mkdirSync(resolve(__dirname, './.data')) + } + + try { + writeFileSync( + this.outputPath, + JSON.stringify(output, null, 2), + 'utf-8', + ) + } + catch (error) { + console.error('Failed to write performance results:', error) + } + } +} diff --git a/packages/core/src/hooks/tests/useControllableState.test.ts b/packages/core/src/hooks/tests/useControllableState.test.ts new file mode 100644 index 000000000..c26be35b5 --- /dev/null +++ b/packages/core/src/hooks/tests/useControllableState.test.ts @@ -0,0 +1,303 @@ +import { useVModel } from '@vueuse/core' +import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { type ComponentInternalInstance, computed, nextTick, ref, type Ref, watch } from 'vue' +import { useControllableStateV3, useControllableStateV5, useControllableStateV6 } from '../useControllableState' +import { PerformanceLogger } from './performance-results' + +// Mock Vue's lifecycle hooks +// eslint-disable-next-line unused-imports/no-unused-vars +let unmountHandler: (() => void) | undefined +vi.mock('vue', async () => { + const actual = await vi.importActual('vue') + return { + ...actual as any, + onBeforeUnmount: vi.fn((fn) => { + unmountHandler = fn + }), + } +}) + +// Update type definitions for controller tests +type Function = (...args: any[]) => any +type PropGetter = (() => T | undefined) & Function +type PropValue = T | undefined +type DefaultValueGetter = (() => T) & Function +type PropType = PropGetter | Exclude, Function> +type DefaultType = DefaultValueGetter | Exclude + +// Add type guard +function isPropGetter(prop: PropType): prop is PropGetter { + return typeof prop === 'function' +} + +// Update the controller function type +type ControllerFn = { + ( + prop: PropType, + onChange: ((value: T) => void) | undefined, + defaultValue: DefaultType, + options?: any + ): Ref +} + +function createControllerTests( + name: string, + useController: ControllerFn, +) { + describe(name, () => { + beforeEach(() => { + vi.useFakeTimers() + unmountHandler = undefined + }) + + afterEach(() => { + vi.restoreAllMocks() + vi.clearAllMocks() + }) + + it('should handle initial values correctly', () => { + const value = useController(undefined, undefined, () => 10) + expect(value.value).toBe(10) + }) + + it('should handle function props correctly', () => { + const propFn = () => 20 + const value = useController(propFn, undefined, () => 10) + expect(value.value).toBe(20) + }) + + it('should handle multiple rapid updates', async () => { + const onChange = vi.fn() + const value = useController(undefined, onChange, () => 0) + + // Sequential updates + value.value = 1 + value.value = 2 + value.value = 3 + + // Process updates + vi.runAllTimers() + await nextTick() + + // Check final state + expect(value.value).toBe(3) + expect(onChange).toHaveBeenLastCalledWith(3) + }) + + it('should handle v-model style updates', async () => { + const vModel = ref(0) + const onChange = vi.fn((v) => { + vModel.value = v + }) + const value = useController(() => vModel.value, onChange, () => 0) + + value.value = 5 + vi.runAllTimers() + await nextTick() + + expect(vModel.value).toBe(5) + expect(value.value).toBe(5) + }) + + it('should measure performance of rapid updates', async () => { + const onChange = vi.fn() + const value = useController(undefined, onChange, () => 0) + const start = performance.now() + + // Initial setup + await nextTick() + vi.runAllTimers() + + // Standard test loop + for (let i = 0; i < 5; i++) { + value.value = i + vi.runAllTimers() + await nextTick() + } + + const end = performance.now() + const executionTime = end - start + + // Log results using PerformanceLogger + PerformanceLogger.logResult( + name, + executionTime, + onChange.mock.calls.length, + ) + + expect(value.value).toBe(4) + }) + + it('should handle external prop changes', async () => { + const externalValue = ref(0) + const onChange = vi.fn() + const value = useController(() => externalValue.value, onChange, () => 0) + + externalValue.value = 10 + vi.runAllTimers() + await nextTick() + + expect(value.value).toBe(10) + }) + + it('should respect controlled vs uncontrolled behavior', () => { + const controlled = useController(() => 5, vi.fn(), () => 0) + const uncontrolled = useController(undefined, vi.fn(), () => 0) + + expect(controlled.value).toBe(5) + expect(uncontrolled.value).toBe(0) + }) + }) +} + +// Fix the createVModelWrapper type signature +function createVModelWrapper( + prop: PropType, + onChange: ((value: T) => void) | undefined, + defaultValue: DefaultType, +): Ref { + const initialValue = typeof defaultValue === 'function' + ? (defaultValue as DefaultValueGetter)() + : defaultValue + const currentValue = ref(initialValue) + const emit = vi.fn((event: string, ...args: any[]) => { + if (event === 'update:modelValue' && onChange) + onChange(args[0]) + }) + + // Create mock instance with proper type handling + const instance = { + emit, + proxy: { + $props: computed(() => ({ + modelValue: isPropGetter(prop) ? prop() : prop, + })), + $emit: emit, + }, + refs: {}, + attrs: {}, + slots: {}, + setupState: {}, + ctx: { + emit, + }, + } as unknown as ComponentInternalInstance + + // Create model with sync between prop and internal value + const model = useVModel( + instance, + 'modelValue' as keyof ComponentInternalInstance, + emit, + { + defaultValue: initialValue as any, + passive: true, // Change to true for better prop sync + }, + ) + + // Sync with prop changes + if (isPropGetter(prop)) { + watch( + () => prop(), + (newVal) => { + if (newVal !== undefined) { + currentValue.value = newVal + model.value = newVal + } + }, + { immediate: true }, // Important for initial sync + ) + } + else if (prop !== undefined) { + currentValue.value = prop + model.value = prop + } + + // eslint-disable-next-line ts/ban-ts-comment + // @ts-ignore + return computed({ + get: () => model.value, + set: (value) => { + currentValue.value = value + model.value = value + }, + }) +} + +// Use explicit type for controllers +const controllers: Record = { + // eslint-disable-next-line ts/ban-ts-comment + // @ts-ignore + useControllableStateV3, + useControllableStateV5, + // eslint-disable-next-line ts/ban-ts-comment + // @ts-ignore + useControllableStateV6, + createVModelWrapper, +} + +// Use type-safe runner calls +createControllerTests('useControllableStateV3', controllers.useControllableStateV3) +createControllerTests('useControllableStateV5', controllers.useControllableStateV5) +createControllerTests('useControllableStateV6', controllers.useControllableStateV6) +createControllerTests('useVModel_VueUse', controllers.createVModelWrapper) + +// Update VueUse specific tests +describe('vueUse useVModel specific tests', () => { + it('should handle v-model correctly', async () => { + const onChange = vi.fn() + const value = createVModelWrapper(undefined, onChange, 0) + + value.value = 5 + await nextTick() + + expect(onChange).toHaveBeenCalledWith(5) + }) + + it('should handle external prop changes with nextTick', async () => { + const externalValue = ref(0) + const onChange = vi.fn() + const value = createVModelWrapper(() => externalValue.value, onChange, 0) + + await nextTick() + expect(value.value).toBe(0) // Initial value + + externalValue.value = 10 + await nextTick() + await nextTick() // Need two ticks for VueUse update to propagate + + expect(value.value).toBe(10) + }) + + // Fix the syntax error in the test + it('should respect controlled vs uncontrolled behavior with async updates', async () => { + const controlled = createVModelWrapper(() => 5, vi.fn(), 0) + const uncontrolled = createVModelWrapper(undefined, vi.fn(), 0) + + await nextTick() + await nextTick() + + expect(controlled.value).toBe(5) + expect(uncontrolled.value).toBe(0) // Fixed missing parenthesis + }) +}) + +// Update performance test to be more flexible +describe('performance Comparison', () => { + it('should compare implementations performance', () => { + const comparison = PerformanceLogger.compareVersions() + + // Remove strict performance assertion and replace with general check + if (comparison.v3Average && comparison.v5Average) { + // Just verify both implementations complete successfully + expect(comparison.v3Average.executionTimeMs).toBeGreaterThan(0) + expect(comparison.v5Average.executionTimeMs).toBeGreaterThan(0) + + // Log relative performance without asserting + comparison.v5Average.executionTimeMs / comparison.v3Average.executionTimeMs + } + }) + + afterAll(() => { + PerformanceLogger.getResults() + }) +}) diff --git a/packages/core/src/hooks/useControllableState.ts b/packages/core/src/hooks/useControllableState.ts index 0f80e1bfc..be1071173 100644 --- a/packages/core/src/hooks/useControllableState.ts +++ b/packages/core/src/hooks/useControllableState.ts @@ -1,4 +1,4 @@ -import { computed, nextTick, type Ref, shallowRef, type UnwrapRef, watch } from 'vue' +import { computed, nextTick, onBeforeUnmount, type Ref, shallowRef, type UnwrapRef, watch } from 'vue' // type NonUndefined = T extends undefined ? never : T @@ -142,3 +142,138 @@ export function useControllableStateV3( return proxy } + +export function useControllableStateV5( + prop: (() => T | undefined) | T | undefined, + onChange: ((value: T) => void) | undefined, + defaultValue: (() => T) | T, +): Ref { + // Safe type checking for functions + const getPropValue = () => { + if (typeof prop === 'function') { + return (prop as () => T | undefined)() + } + return prop + } + + const getDefaultValue = () => { + if (typeof defaultValue === 'function') { + return (defaultValue as () => T)() + } + return defaultValue + } + + const valueRef = shallowRef(getPropValue() ?? getDefaultValue()) + + // Optimize update batching + const batchedUpdates = new Set() + let isUpdating = false + let rafId: number | undefined + + const scheduleUpdate = (newValue: T) => { + batchedUpdates.add(newValue) + + if (!isUpdating) { + isUpdating = true + rafId = requestAnimationFrame(() => { + const lastValue = Array.from(batchedUpdates).pop()! + if (lastValue !== valueRef.value) { + valueRef.value = lastValue + onChange?.(lastValue) + } + batchedUpdates.clear() + isUpdating = false + rafId = undefined + }) + } + } + + // Cleanup + onBeforeUnmount(() => { + if (rafId !== undefined) + cancelAnimationFrame(rafId) + }) + + // Watch external changes with proper type handling + if (prop !== undefined) { + watch( + () => getPropValue(), + (newVal) => { + if (newVal !== undefined && newVal !== valueRef.value) { + scheduleUpdate(newVal) + } + }, + { flush: 'sync' }, + ) + } + + // Return computed with optimized setter + return computed({ + get: () => valueRef.value, + set: scheduleUpdate, + }) +} + +export function useControllableStateV6( + prop: (() => T | undefined) | T, + onChange: ((value: T) => void) | undefined, + defaultValue: (() => T) | T, +): Ref { + // Single WeakMap for all caching needs + const cache = new WeakMap() + + // Optimized type checking + const isCallable = (value: unknown): value is () => V => { + if (typeof value !== 'object') + return typeof value === 'function' + const cached = cache.get(value as object) + if (cached?.isFunction !== undefined) + return cached.isFunction + const isFunc = typeof value === 'function' + cache.set(value as object, { isFunction: isFunc }) + return isFunc + } + + // Ultra-fast value resolution + const getValue = (value: (() => V) | V): V => + isCallable(value) ? value() : value + + // Initialize state + const valueRef = shallowRef(getValue(prop) ?? getValue(defaultValue)) + const updates = new Set() + let isProcessing = false + + // Simplified update processor + const processUpdates = () => { + const values = Array.from(updates) + updates.clear() + + if (values.length > 0) { + const newValue = values[values.length - 1] + valueRef.value = newValue + onChange?.(newValue) + } + + isProcessing = false + } + + // Direct update scheduler + const scheduleUpdate = (value: T) => { + updates.add(value) + if (!isProcessing) { + isProcessing = true + queueMicrotask(processUpdates) + } + } + + // Watch with minimal overhead + if (isCallable(prop)) { + watch(prop, next => next !== undefined && scheduleUpdate(next), { flush: 'sync' }, + ) + } + + return computed({ + get: () => valueRef.value, + set: scheduleUpdate, + }) +} diff --git a/packages/core/src/slider/SliderRoot.ts b/packages/core/src/slider/SliderRoot.ts index 5849b4a05..a1e031bb9 100644 --- a/packages/core/src/slider/SliderRoot.ts +++ b/packages/core/src/slider/SliderRoot.ts @@ -3,7 +3,7 @@ import type { EmitsToHookProps, LooseRequired, PrimitiveDefaultProps, RadixPrimi import { computed, type HTMLAttributes, type MaybeRefOrGetter, type Ref, type UnwrapRef } from 'vue' import { createCollection } from '../collection/index.ts' import { type Direction, useDirection } from '../direction/index.ts' -import { createContext, type MutableRefObject, useControllableStateV3, useRef, type useSize } from '../hooks/index.ts' +import { createContext, type MutableRefObject, useControllableStateV5, useRef, type useSize } from '../hooks/index.ts' import { clamp, getDecimalCount, isNumber, mergePrimitiveAttrs, roundValue } from '../shared/index.ts' import { getClosestValueIndex, getNextSortedValues, hasMinStepsBetweenValues, linearScale } from './utils.ts' @@ -31,6 +31,7 @@ export interface SliderRootProps { value?: number[] defaultValue?: number[] inverted?: boolean + modelValue?: number[] // Add this } export const DEFAULT_SLIDER_ROOT_PROPS = { @@ -47,6 +48,7 @@ export const DEFAULT_SLIDER_ROOT_PROPS = { export type SliderRootEmits = { 'update:value': [value: number[]] 'valueCommit': [value: number[]] + 'update:modelValue': [value: number[]] // Add this } export interface SliderContext { @@ -77,7 +79,7 @@ export const [provideSliderOrientationContext, useSliderOrientationContext] = cr export interface UseSliderRootProps extends EmitsToHookProps { el?: MutableRefObject - value?: () => number[] | undefined + value?: (() => number[] | undefined) | number[] defaultValue?: () => number[] name?: () => string | undefined disabled?: () => boolean | undefined @@ -88,6 +90,7 @@ export interface UseSliderRootProps extends EmitsToHookProps { step?: () => number minStepsBetweenThumbs?: () => number inverted?: boolean + modelValue?: (() => number[] | undefined) | number[] } export function useSliderRoot(props: UseSliderRootProps): RadixPrimitiveReturns { @@ -113,13 +116,11 @@ export function useSliderRoot(props: UseSliderRootProps): RadixPrimitiveReturns const thumbs: SliderContext['thumbs'] = new Set() const valueIndexToChangeRef = useRef(0) - // TODO: is not reactive - const values = useControllableStateV3( - props.value, + const values = useControllableStateV5( + props.modelValue ?? props.value, (v) => { - const _thumbs = Array.from(thumbs) - _thumbs[valueIndexToChangeRef.value]?.focus() - props.onUpdateValue?.(v as number[]) + props.onUpdateModelValue?.(v) + props.onUpdateValue?.(v) }, defaultValue, ) diff --git a/packages/core/src/slider/stories/SmallSteps.vue b/packages/core/src/slider/stories/SmallSteps.vue index 8a00aa4df..5a9526e87 100644 --- a/packages/core/src/slider/stories/SmallSteps.vue +++ b/packages/core/src/slider/stories/SmallSteps.vue @@ -8,11 +8,11 @@ const value = shallowRef([0.1])