-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
fix(reactivity): fix dep memory leak #7827
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,12 +143,23 @@ function cleanupEffect(effect: ReactiveEffect) { | |
const { deps } = effect | ||
if (deps.length) { | ||
for (let i = 0; i < deps.length; i++) { | ||
deps[i].delete(effect) | ||
removeEffectFromDep(deps[i], effect) | ||
} | ||
deps.length = 0 | ||
} | ||
} | ||
|
||
export function removeEffectFromDep(dep: Dep, effect: ReactiveEffect) { | ||
dep.delete(effect) | ||
if (dep.target && dep.size === 0) { | ||
const depsMap = targetMap.get(dep.target) | ||
if (depsMap) { | ||
depsMap.delete(dep.key) | ||
} | ||
dep.target = dep.key = null | ||
} | ||
} | ||
|
||
Comment on lines
+152
to
+162
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered using a However, in the most common cases, the |
||
export interface DebuggerOptions { | ||
onTrack?: (event: DebuggerEvent) => void | ||
onTrigger?: (event: DebuggerEvent) => void | ||
|
@@ -252,7 +263,7 @@ export function track(target: object, type: TrackOpTypes, key: unknown) { | |
} | ||
let dep = depsMap.get(key) | ||
if (!dep) { | ||
depsMap.set(key, (dep = createDep())) | ||
depsMap.set(key, (dep = createDep(target, key))) | ||
} | ||
|
||
const eventInfo = __DEV__ | ||
|
@@ -383,19 +394,19 @@ export function trigger( | |
} | ||
} | ||
if (__DEV__) { | ||
triggerEffects(createDep(effects), eventInfo) | ||
triggerEffects(new Set(effects), eventInfo) | ||
} else { | ||
triggerEffects(createDep(effects)) | ||
triggerEffects(new Set(effects)) | ||
} | ||
} | ||
} | ||
|
||
export function triggerEffects( | ||
dep: Dep | ReactiveEffect[], | ||
dep: Set<ReactiveEffect>, | ||
debuggerEventExtraInfo?: DebuggerEventExtraInfo | ||
) { | ||
// spread into array for stabilization | ||
const effects = isArray(dep) ? dep : [...dep] | ||
const effects = [...dep] | ||
Comment on lines
+397
to
+409
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes were inspired by #6185. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can keep the signature of triggerEffects, then create the Set inside the triggerEffects if needed. |
||
for (const effect of effects) { | ||
if (effect.computed) { | ||
triggerEffect(effect, debuggerEventExtraInfo) | ||
|
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.
I wasn't sure if there was a reason why the properties
w
andn
use single-letter names. I've usedtarget
andkey
here, but they can easily be changed tot
andk
if required.