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.

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

fix: #83、exit delay #84

Merged
merged 16 commits into from
Feb 27, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "vue-motion",
"version": "0.10.0",
"version": "0.11.0-beta.4",
"private": true,
"packageManager": "pnpm@9.15.0+sha1.8bfdb6d72b4d5fdf87d21d27f2bfbe2b21dd2629",
"description": "",
Expand Down
2 changes: 1 addition & 1 deletion packages/motion/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "motion-v",
"version": "0.10.0",
"version": "0.11.0-beta.4",
"description": "",
"author": "",
"license": "MIT",
Expand Down
4 changes: 3 additions & 1 deletion packages/motion/src/components/LayoutGroup.vue
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
<script setup lang="ts">
import { type LayoutGroupProps, useLayoutGroupProvider } from './use-layout-group'

const props = defineProps<LayoutGroupProps>()
const props = withDefaults(defineProps<LayoutGroupProps>(), {
inherit: true,
})
const { forceRender, key } = useLayoutGroupProvider(props)
</script>

Expand Down
49 changes: 25 additions & 24 deletions packages/motion/src/components/animate-presence/AnimatePresence.vue
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,19 @@ import { mountedStates } from '@/state'
import { doneCallbacks, provideAnimatePresence, removeDoneCallback } from '@/components/presence'
import type { AnimatePresenceProps } from './types'
import { usePopLayout } from './use-pop-layout'
import { frame } from 'framer-motion/dom'
import { delay } from '@/utils/delay'

// 定义组件选项
defineOptions({
name: 'AnimatePresence',
inheritAttrs: true,
})

// 设置Props默认值
const props = withDefaults(defineProps<AnimatePresenceProps>(), {
mode: 'sync',
initial: true,
multiple: false,
unwrapElement: false,
anchorX: 'left',
})

const presenceContext = {
Expand All @@ -29,34 +28,37 @@ provideAnimatePresence(presenceContext)
onMounted(() => {
presenceContext.initial = undefined
})
const { addPopStyle, removePopStyle, styles } = usePopLayout(props)

// 处理元素进入动画
function enter(el: HTMLElement) {
const state = mountedStates.get(el)
if (!state) {
return
}
removePopStyle(state)
state.isVShow = true
removeDoneCallback(el)
state.setActive('exit', false)
/**
* Delay to ensure animations read the latest state before triggering.
* This allows the animation system to capture updated values after component updates.
*/
delay(() => {
state.setActive('exit', false)
})
}

const { addPopStyle, removePopStyle, styles } = usePopLayout(props)

const exitDom = new Map<Element, boolean>()

onUnmounted(() => {
exitDom.clear()
})
// 处理元素退出动画
function exit(el: Element, done: VoidFunction) {
let state = mountedStates.get(el)
if (!state) {
if (!props.unwrapElement) {
return done()
}
if (props.unwrapElement) {
el = el.firstElementChild as Element
state = mountedStates.get(el)
}
const state = mountedStates.get(el)
if (!state) {
return done()
}
exitDom.set(el, true)
removeDoneCallback(el)
Expand All @@ -77,20 +79,23 @@ function exit(el: Element, done: VoidFunction) {
state.willUpdate('done')
}
else {
frame.render(() => {
removePopStyle(state)
})
removePopStyle(state)
}
done()

if (!el?.isConnected) {
if (!el.isConnected) {
state.unmount(true)
}
}
}
doneCallbacks.set(el, doneCallback)
el.addEventListener('motioncomplete', doneCallback)
state.setActive('exit', true)
/**
* Delay to ensure animations read the latest state before triggering.
* This allows the animation system to capture updated values after component updates.
*/
delay(() => {
state.setActive('exit', true)
})
}

const transitionProps = computed(() => {
Expand All @@ -117,7 +122,3 @@ const transitionProps = computed(() => {
<slot />
</component>
</template>

<style scoped>

</style>
1 change: 1 addition & 0 deletions packages/motion/src/components/animate-presence/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@ export interface AnimatePresenceProps {
custom?: any

Choose a reason for hiding this comment

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

The custom property is currently typed as any, which can lead to potential issues with type safety and maintainability. It's recommended to replace any with a more specific type or an interface that accurately describes the expected structure of custom. This change would leverage TypeScript's type checking to prevent runtime errors and improve code quality.

onExitComplete?: VoidFunction
unwrapElement?: boolean
anchorX?: 'left' | 'right'
}
20 changes: 11 additions & 9 deletions packages/motion/src/components/animate-presence/use-pop-layout.ts
Original file line number Diff line number Diff line change
@@ -1,41 +1,43 @@
import { useMotionConfig } from '@/components/motion-config/context'
import type { AnimatePresenceProps } from './types'
import type { MotionState } from '@/state'
import { frame } from 'framer-motion/dom'

export function usePopLayout(props: AnimatePresenceProps) {
const styles = new WeakMap<MotionState, HTMLStyleElement>()
const config = useMotionConfig()

function addPopStyle(state: MotionState) {
if (props.mode !== 'popLayout')
return
const parent = state.element.offsetParent
const parentWidth
= parent instanceof HTMLElement ? parent.offsetWidth || 0 : 0
const size = {
height: state.element.offsetHeight || 0,
width: state.element.offsetWidth || 0,
top: state.element.offsetTop,
left: state.element.offsetLeft,
right: 0,
}
size.right = parentWidth - size.width - size.left
const x = props.anchorX === 'left' ? `left: ${size.left}` : `right: ${size.right}`

state.element.dataset.motionPopId = state.id
const style = document.createElement('style')
if (config.value.nonce) {
style.nonce = config.value.nonce
}
styles.set(state, style)
document.head.appendChild(style)
Comment on lines 26 to 31

Choose a reason for hiding this comment

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

Creating and appending a new <style> element for each component state can lead to performance degradation, especially in large applications. This approach increases the number of DOM elements significantly, which can slow down the page rendering and increase memory usage.

Recommendation: Consider using a single <style> element managed by usePopLayout or a similar context provider. This element could aggregate all dynamic styles, which would reduce the number of DOM manipulations and improve performance. Use class names or CSS variables to apply styles dynamically to the elements.

style.textContent = `
[data-motion-pop-id="${state.id}"] {
animation: pop 0.3s ease-in-out;
}
`
if (style.sheet) {
style.sheet.insertRule(`
[data-motion-pop-id="${state.id}"] {
position: absolute !important;
width: ${size.width}px !important;
height: ${size.height}px !important;
top: ${size.top}px !important;
left: ${size.left}px !important;
}
${x}px !important;
}
`)
Comment on lines 33 to 41

Choose a reason for hiding this comment

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

The use of insertRule with template literals that include dynamic content (e.g., state.id, size.width, etc.) poses a risk of CSS injection if the values are not properly sanitized. This can lead to security vulnerabilities where malicious content could be executed.

Recommendation: Ensure all dynamic values inserted into CSS rules are properly sanitized to prevent CSS injection. Alternatively, consider safer methods to apply styles, such as setting styles directly on elements via JavaScript, which avoids the complexity and risks associated with dynamic CSS rules.

}
}
Expand All @@ -45,7 +47,7 @@ export function usePopLayout(props: AnimatePresenceProps) {
if (!style)
return
styles.delete(state)
requestIdleCallback(() => {
frame.render(() => {
document.head.removeChild(style)
})
}
Expand Down
1 change: 1 addition & 0 deletions packages/motion/src/components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ export * from './animate-presence'
export * from './motion-config'
export * from './reorder'
Comment on lines 3 to 4

Choose a reason for hiding this comment

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

Using export * from modules (./motion-config and ./reorder) can lead to namespace pollution and makes it difficult to track where specific exports are coming from. This can complicate maintenance and debugging. Consider explicitly listing which members to export to improve code clarity and maintainability.

Recommended Change:

// Example for motion-config
export { specificConfig, anotherConfig } from './motion-config'
// Adjust for actual exported members

export { default as RowValue } from './RowValue.vue'
export { mountedStates } from '@/state'

Choose a reason for hiding this comment

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

The use of an absolute path (@/state) for importing mountedStates can lead to issues with module resolution, especially if the project's configuration changes or in different environments. Consider using relative paths or ensuring that the module resolution configuration is robust across different environments.

Recommended Change:

// If possible, convert to a relative path
export { mountedStates } from '../../state'
// Adjust the path as necessary based on the actual structure

5 changes: 4 additions & 1 deletion packages/motion/src/components/motion/NameSpace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { DefineComponent, ExtractPropTypes, ExtractPublicPropTypes, Intrins
import { defineComponent, h } from 'vue'
import Motion from './Motion.vue'
import type { MotionProps } from './Motion.vue'
import type { MotionHTMLAttributes } from '@/types'

type ComponentProps<T> = T extends DefineComponent<
ExtractPropTypes<infer Props>,
Comment on lines 7 to 8

Choose a reason for hiding this comment

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

The type definition for ComponentProps uses advanced TypeScript features which can make the codebase harder to maintain and understand for developers not familiar with these features. Consider simplifying these types or providing detailed documentation to aid in maintainability.

Expand All @@ -15,7 +16,9 @@ type MotionComponentProps = {
}
type MotionKeys = keyof MotionComponentProps

interface MotionNameSpace extends Record<keyof IntrinsicElementAttributes, DefineComponent<MotionProps<keyof IntrinsicElementAttributes, unknown>>> {
type MotionNameSpace = {
[K in keyof IntrinsicElementAttributes]: DefineComponent<MotionProps<K, unknown> & MotionHTMLAttributes<K>>
} & {
create: MotionComponentProps['create']
}
Comment on lines +19 to 23

Choose a reason for hiding this comment

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

The MotionNameSpace type definition is complex, utilizing mapped and intersection types extensively. This could lead to difficulties in maintenance and extension of the codebase. Simplifying these definitions or splitting them into smaller, more manageable parts could improve code clarity and maintainability.


Expand Down
19 changes: 12 additions & 7 deletions packages/motion/src/features/layout/layout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,22 @@ export class LayoutFeature extends Feature {
}

Choose a reason for hiding this comment

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

The update method is currently empty. If this method is intended to be overridden or is essential for interface compliance, consider adding a comment to clarify its purpose. If it's not needed, you might want to remove it to avoid confusion and maintain cleaner code.


didUpdate() {
if (this.state.options.layout || this.state.options.layoutId)
if (this.state.options.layout || this.state.options.layoutId || this.state.options.drag)
this.state.visualElement.projection?.root?.didUpdate()
}

mount() {
const options = this.state.options
const layoutGroup = this.state.options.layoutGroup
if (options.layout || options.layoutId || options.drag) {
if (options.layout || options.layoutId) {
const projection = this.state.visualElement.projection
if (projection) {
projection.promote()
layoutGroup?.group?.add(projection)
}
this.didUpdate()
globalProjectionState.hasEverUpdated = true

Choose a reason for hiding this comment

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

Directly modifying a global state (globalProjectionState.hasEverUpdated = true) can lead to unintended side effects if this state is shared across multiple components or features. Consider encapsulating this state change within a method that can handle side effects or ensure that the state change is appropriate at the time of modification.

}
this.didUpdate()
}

beforeUnmount(): void {
Expand All @@ -55,10 +55,15 @@ export class LayoutFeature extends Feature {
const layoutGroup = this.state.options.layoutGroup
const projection = this.state.visualElement.projection

if (layoutGroup?.group && projection) {
layoutGroup.group.remove(projection)
}
else {
if (projection) {
if (layoutGroup?.group) {
layoutGroup.group.remove(projection)
}

// Check lead's animation progress, if it exists, skip update to prevent lead from jumping
if (projection.getStack()?.lead?.animationProgress) {
return
}
this.didUpdate()
}
}
Expand Down
1 change: 1 addition & 0 deletions packages/motion/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export * from 'framer-motion/dom'
export { addScaleCorrector } from 'framer-motion/dist/es/projection/styles/scale-correction.mjs'

Choose a reason for hiding this comment

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

The export from a deeply nested path within framer-motion (line 2) risks future compatibility issues if the internal structure of the dependency changes. It's safer to rely on top-level exports provided by the package to avoid potential breaks during updates.

Suggested Change:
Contact the maintainers of framer-motion to see if a more stable path or method can be provided for this import.

export { motionValue as useMotionValue } from 'framer-motion/dom'
export * from './components'
Comment on lines 1 to 4

Choose a reason for hiding this comment

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

Using export * (seen in lines 1 and 4) can lead to namespace pollution and complicates tree-shaking, potentially resulting in larger bundle sizes. It's recommended to explicitly list which modules or components are being exported to improve code clarity and maintainability.

Suggested Change:

// Instead of export * from 'module/path'
export { specificModule1, specificModule2 } from 'module/path';

export { default as LayoutGroup } from './components/LayoutGroup.vue'
Expand Down
3 changes: 0 additions & 3 deletions packages/motion/src/state/animate-updates.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,11 @@ export function animateUpdates(
this.target = { ...this.baseTarget }
const animationOptions: Record<string, $Transition> = {}
const transition = { ...this.options.transition }
Comment on lines 41 to 42

Choose a reason for hiding this comment

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

The use of the spread operator to clone this.options.transition into transition can lead to performance issues if this.options.transition is a large object. Each time animateUpdates is called, a new object is created which can increase memory usage and slow down execution if done frequently in a performance-sensitive environment.

Recommendation:
Consider checking the necessity of cloning this.options.transition every time, especially if it is not modified in most calls. If modifications are rare, you might use the original object directly or use other methods to handle immutability more efficiently.


// 处理直接动画或状态动画
if (directAnimate)
resolveDirectAnimation.call(this, directAnimate, directTransition, animationOptions)
else
resolveStateAnimation.call(this, controlActiveState, animationOptions)

const factories = createAnimationFactories.call(this, prevTarget, animationOptions, controlDelay)
const { getChildAnimations, childAnimations } = setupChildAnimations.call(this, transition, controlActiveState, isFallback)

Expand Down Expand Up @@ -120,7 +118,6 @@ function createAnimationFactories(
Object.keys(this.target).forEach((key: any) => {
if (!hasChanged(prevTarget[key], this.target[key]))
return

this.baseTarget[key] ??= style.get(this.element, key) as string
const keyValue = this.target[key] === 'none' ? transformResetValue[key] : this.target[key]

Choose a reason for hiding this comment

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

The handling of this.target[key] === 'none' to decide the animation value introduces a specific logic that treats 'none' as a special case. This can be problematic if 'none' has different implications in various parts of your application or if the properties being animated can legitimately have 'none' as a valid value.

Recommendation:
Ensure that the use of 'none' as a special value is consistent and well-documented across your application. Consider defining a more explicit way or using a specific function to handle these cases to avoid ambiguity and potential bugs in the animation behavior.

const targetTransition = animationOptions[key]
Expand Down
48 changes: 23 additions & 25 deletions packages/motion/src/state/motion-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,38 +174,36 @@ export class MotionState {
// Unmount motion state and optionally unmount children
// Handles unmounting in the correct order based on component tree
unmount(unMountChildren = false) {
/**
* Unlike React, within the same update cycle, the execution order of unmount and mount depends on the component's order in the component tree.
* Here we delay unmount for components with layoutId to ensure unmount executes after mount for layout animations.
*/
const shouldDelay = this.options.layoutId && !mountedLayoutIds.has(this.options.layoutId)
const unmount = () => {
mountedStates.delete(this.element)
this.featureManager.unmount()
if (unMountChildren && !shouldDelay) {
frame.render(() => {
this.visualElement?.unmount()
})
if (unMountChildren) {
Array.from(this.children).reverse().forEach(this.unmountChild)
}
else {

const unmountState = () => {
this.parent?.children?.delete(this)
mountedStates.delete(this.element)
this.featureManager.unmount()
this.visualElement?.unmount()
}
// Recursively unmount children in child-to-parent order
if (unMountChildren) {
const unmountChild = (child: MotionState) => {
child.unmount(true)
child.children?.forEach(unmountChild)
}
Array.from(this.children).forEach(unmountChild)
// Delay unmount if needed for layout animations
if (shouldDelay) {
Promise.resolve().then(unmountState)
Comment on lines +194 to +195

Choose a reason for hiding this comment

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

The use of Promise.resolve().then(unmountState) to delay the unmount operation introduces a potential issue where errors thrown in the unmountState function are not caught, leading to unhandled promise rejections. This can cause silent failures in the application.

Recommendation: Wrap the unmountState call within the promise chain with a .catch() block to handle possible errors gracefully. For example:

Promise.resolve().then(unmountState).catch(error => console.error('Failed to unmount state:', error));

}
else {
unmountState()
}
this.parent?.children?.delete(this)
}

// Delay unmount if needed for layout animations
if (shouldDelay) {
Promise.resolve().then(() => {
unmount()
})
}
else {
unmount()
}
unmount()
}

private unmountChild(child: MotionState) {
child.unmount(true)
}

// Called before updating, executes in parent-to-child order
Expand Down Expand Up @@ -236,7 +234,7 @@ export class MotionState {
})
if (isAnimate) {
this.animateUpdates({
isFallback: !isActive,
isFallback: !isActive && name !== 'exit' && this.visualElement.isControllingVariants,
})
}
}
Comment on lines 234 to 240

Choose a reason for hiding this comment

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

The recursive call to setActive for child components in line 234 does not handle potential errors that might occur in the child's setActive method. This can lead to unhandled exceptions if a child component encounters an issue during its state update.

Recommendation: Consider adding error handling around the recursive call to ensure stability. For example:

this.visualElement.variantChildren?.forEach((child) => {
  try {
    ((child as any).state as MotionState).setActive(name, isActive, false);
  } catch (error) {
    console.error('Error updating active state for child:', error);
  }
});

Expand Down
2 changes: 1 addition & 1 deletion packages/motion/src/state/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export function removeItem<T>(array: T[], item: T) {
}

export function getOptions(options: $Transition, key: string): $Transition {
return options[key as any] ? { ...options, ...options[key as any] } : { ...options }
return options[key as any] ? { ...options, ...options[key as any], [key]: undefined } : { ...options }
Comment on lines 51 to +52

Choose a reason for hiding this comment

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

The function getOptions uses object spreading in a potentially inefficient and unsafe manner. If options[key] is a large object, spreading it can be costly in terms of performance. Additionally, the type casting key as any could lead to runtime errors if key is not a valid property, potentially accessing undefined properties without checks.

Recommendation:

  • Consider checking if key is a valid property before accessing it to avoid runtime errors.
  • If performance is a concern and options[key] can be large, consider alternative methods to manipulate objects that do not involve spreading large objects.

}

export function isCssVar(name: string) {

Choose a reason for hiding this comment

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

The function isCssVar uses optional chaining (name?.startsWith('--')) which is unnecessary since name is a required parameter of type string. This could lead to confusion about the function's expectations regarding its inputs and reduce code clarity.

Recommendation:

  • Remove the optional chaining to enforce the expectation that name should always be a string, enhancing the clarity and maintainability of the function.

Expand Down
7 changes: 5 additions & 2 deletions packages/motion/src/types/motion-values.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { MotionValue } from 'framer-motion/dom'
import type { Options } from './state'
import type { AriaAttributes, Events, SVGAttributes } from 'vue'
import type { ElementType, Options } from './state'
import type { AriaAttributes, Events, IntrinsicElementAttributes, SVGAttributes } from 'vue'

type EventHandlers<E> = {
[K in keyof E]?: E[K] extends (...args: any) => any ? E[K] : (payload: E[K]) => void;
Expand Down Expand Up @@ -101,3 +101,6 @@ export interface SVGAttributesWithMotionValues {
export type SetMotionValueType<T, Keys extends keyof T> = {
[K in keyof T]: K extends Keys ? SVGAttributesAsMotionValues : T[K]
}

type IntrinsicElementAttributesAsMotionValues = SetMotionValueType<IntrinsicElementAttributes, keyof SVGAttributesWithMotionValues>
export type MotionHTMLAttributes<C extends ElementType> = Omit<IntrinsicElementAttributesAsMotionValues[C], keyof Options | 'style' | 'as' | 'asChild'>
Loading
Loading