-
Notifications
You must be signed in to change notification settings - Fork 11
test(): fix test error reporting #4828
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
base: v3
Are you sure you want to change the base?
Changes from 3 commits
bdab057
1b44b42
0a334bc
6d8ff39
dd5c9f5
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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -19,6 +19,7 @@ import { ResolveOptions, resolveData } from "./resolveData.js"; | |||||||
| import { resolveDataStore } from "./resolveDataStore.js"; | ||||||||
| import type { | ||||||||
| AsyncPropertyEntry, | ||||||||
| Dispose, | ||||||||
| RouteNode, | ||||||||
| RuntimeBrick, | ||||||||
| RuntimeContext, | ||||||||
|
|
@@ -71,6 +72,7 @@ export class DataStore<T extends DataStoreType = "CTX"> { | |||||||
| private readonly rendererContext?: RendererContext; | ||||||||
| private routeMap = new WeakMap<RouteConf, Set<string>>(); | ||||||||
| private routeStackMap = new WeakMap<RouteConf, Set<PendingStackItem>>(); | ||||||||
| private disposableMap = new Map<string, Dispose[]>(); | ||||||||
|
|
||||||||
| // 把 `rendererContext` 放在参数列表的最后,并作为可选,以减少测试文件的调整 | ||||||||
| constructor( | ||||||||
|
|
@@ -360,12 +362,14 @@ export class DataStore<T extends DataStoreType = "CTX"> { | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| onChange(dataName: string, listener: EventListener): () => void { | ||||||||
| onChange(dataName: string, listener: EventListener): Dispose { | ||||||||
| const eventTarget = this.data.get(dataName)?.eventTarget; | ||||||||
| eventTarget?.addEventListener(this.changeEventType, listener); | ||||||||
| return () => { | ||||||||
| const disposable = () => { | ||||||||
| eventTarget?.removeEventListener(this.changeEventType, listener); | ||||||||
| }; | ||||||||
| this.addDisposable(dataName, disposable); | ||||||||
| return disposable; | ||||||||
| } | ||||||||
|
|
||||||||
| async waitFor(dataNames: string[] | Set<string>): Promise<void> { | ||||||||
|
|
@@ -509,7 +513,6 @@ export class DataStore<T extends DataStoreType = "CTX"> { | |||||||
| }; | ||||||||
|
|
||||||||
| if (resolvePolicy === "lazy") { | ||||||||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||||||||
| const { trigger } = dataConf.resolve!; | ||||||||
| if ( | ||||||||
| trigger && | ||||||||
|
|
@@ -539,7 +542,7 @@ export class DataStore<T extends DataStoreType = "CTX"> { | |||||||
| ); | ||||||||
| !load && (newData.deps = [...deps]); | ||||||||
| for (const dep of deps) { | ||||||||
| this.onChange( | ||||||||
| const disposable = this.onChange( | ||||||||
|
||||||||
| dep, | ||||||||
| this.batchAddListener(() => { | ||||||||
| newData.useResolve = trackConditionalResolve | ||||||||
|
|
@@ -556,6 +559,7 @@ export class DataStore<T extends DataStoreType = "CTX"> { | |||||||
| } | ||||||||
| }, dataConf) | ||||||||
| ); | ||||||||
| this.addDisposable(dataConf.name, disposable); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -579,6 +583,15 @@ export class DataStore<T extends DataStoreType = "CTX"> { | |||||||
| return true; | ||||||||
| } | ||||||||
|
|
||||||||
| private addDisposable(name: string, disposable: Dispose) { | ||||||||
| const existingDisposables = this.disposableMap.get(name); | ||||||||
| if (existingDisposables) { | ||||||||
| existingDisposables.push(disposable); | ||||||||
| } else { | ||||||||
| this.disposableMap.set(name, [disposable]); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * For sub-routes to be incrementally rendered, | ||||||||
| * dispose their data and pending tasks. | ||||||||
|
|
@@ -589,6 +602,13 @@ export class DataStore<T extends DataStoreType = "CTX"> { | |||||||
| if (names !== undefined) { | ||||||||
| for (const name of names) { | ||||||||
| this.data.delete(name); | ||||||||
| const disposables = this.disposableMap.get(name); | ||||||||
| if (disposables) { | ||||||||
| for (const disposable of disposables) { | ||||||||
| disposable(); | ||||||||
| } | ||||||||
|
||||||||
| } | |
| } | |
| disposables.length = 0; |
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.
disposableMap 中重复存储同一 Dispose,可能导致长期悬挂引用
目前同一个
Dispose会被存储两次:onChange(dataName, listener)内部调用this.addDisposable(dataName, disposable);(key 为依赖名dataName)。dataConf.track分支中再次调用this.addDisposable(dataConf.name, disposable);(key 为当前 data 名)。在路由销毁逻辑中,
disposeDataInRoutes只按将要删除的 data 名(来自routeMap)去查询disposableMap.get(name)并执行其中的 disposables,然后删除该 key。这样会产生两个问题:Dispose[]永远不会被disposeDataInRoutes删除,即使这些Dispose已经在以dataConf.name为 key 的数组中被执行过,仍然会在disposableMap中保留,持有闭包引用,长期运行有内存上涨风险。disposable被存入两个数组,遇到同时销毁依赖和被依赖 data 的场景时会被调用两次(虽然 DOM/EventTarget 层面是幂等的,但这是不必要的重复工作,也让状态变得更难推理)。建议把“订阅登记”和“所有者”解耦,避免重复存储同一个
Dispose,比如采取更简单的策略:让
onChange仅负责注册监听并返回Dispose,不在内部调用addDisposable:onChange(dataName: string, listener: EventListener): Dispose { const eventTarget = this.data.get(dataName)?.eventTarget; eventTarget?.addEventListener(this.changeEventType, listener);const disposable = () => {
eventTarget?.removeEventListener(this.changeEventType, listener);
};
return disposable;
}