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

ci: disposing unit should not emit error #2529

Merged
merged 13 commits into from
Jun 15, 2024
24 changes: 22 additions & 2 deletions e2e/disposing/disposing.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,30 @@ test('no error on constructing and disposing', async ({ page }) => {
await page.waitForTimeout(2000);

await page.evaluate(() => window.E2EControllerAPI.loadDefaultSheet());
await page.waitForTimeout(2000); // wait for long enough to let the GC do its job
await page.waitForTimeout(2000);

await page.evaluate(() => window.E2EControllerAPI.disposeUniver());
await page.waitForTimeout(2000); // wait for long enough to let the GC do its job
await page.waitForTimeout(2000);

expect(errored).toBeFalsy();
});

test('no error when dispose a unit', async ({ page }) => {
let errored = false;

page.on('pageerror', (error) => {
console.error('Page error:', error);
errored = true;
});

await page.goto('http:https://localhost:3000/sheets/');
await page.waitForTimeout(2000);

await page.evaluate(() => window.E2EControllerAPI.loadAndRelease(1));
await page.waitForTimeout(2000);

await page.evaluate(() => window.E2EControllerAPI.loadAndRelease(2));
await page.waitForTimeout(2000);

expect(errored).toBeFalsy();
});
6 changes: 4 additions & 2 deletions e2e/memory/memory.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { expect, test } from '@playwright/test';
// examples/src/plugins/debugger/controllers/e2e/e2e-memory.controller.ts
export interface IE2EControllerAPI {
loadAndRelease(id: number): Promise<void>;
loadDefaultSheet(): Promise<void>;
disposeUniver(): Promise<void>;
}

Expand All @@ -33,6 +34,8 @@ declare global {
}
}

const MAX_MEMORY_OVERFLOW = 2_500_000;

test('memory', async ({ page }) => {
await page.goto('http:https://localhost:3000/sheets/');
await page.waitForTimeout(2000);
Expand All @@ -49,9 +52,8 @@ test('memory', async ({ page }) => {
const memoryAfterSecondLoad = (await getMetrics(page)).JSHeapUsedSize;
console.log('Memory after second load:', memoryAfterSecondLoad);

// max overflow for 3MB
const notLeaking = (memoryAfterSecondLoad <= memoryAfterFirstLoad)
|| (memoryAfterSecondLoad - memoryAfterFirstLoad <= 2_500_000);
|| (memoryAfterSecondLoad - memoryAfterFirstLoad <= MAX_MEMORY_OVERFLOW);
expect(notLeaking).toBeTruthy();
});

Expand Down
35 changes: 26 additions & 9 deletions packages/core/src/services/instance/instance.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,21 +119,28 @@ export class UniverInstanceService extends Disposable implements IUniverInstance
};
}

private readonly _currentUnits$ = new BehaviorSubject<{ [type: UnitType]: Nullable<UnitModel> }>({});
private _currentUnits = new Map<UnitType, Nullable<UnitModel>>();
private readonly _currentUnits$ = new BehaviorSubject<Map<UnitType, Nullable<UnitModel>>>(this._currentUnits);
readonly currentUnits$ = this._currentUnits$.asObservable();
getCurrentTypeOfUnit$<T>(type: number): Observable<Nullable<T>> {
return this.currentUnits$.pipe(map((units) => units[type] ?? null), distinctUntilChanged()) as Observable<Nullable<T>>;
return this.currentUnits$.pipe(map((units) => units.get(type) ?? null), distinctUntilChanged()) as Observable<Nullable<T>>;
}

getCurrentUnitForType<T>(type: UnitType): Nullable<T> {
return this._currentUnits$.getValue()[type] as Nullable<T>;
return this._currentUnits.get(type) as Nullable<T>;
}

setCurrentUnitForType(unitId: string): void {
const result = this._getUnitById(unitId);
if (!result) throw new Error(`[UniverInstanceService]: no document with unitId ${unitId}!`);

this._currentUnits$.next({ ...this._currentUnits$.getValue(), [result[1]]: result[0] });
this._currentUnits.set(result[1], result[0]);
this._currentUnits$.next(this._currentUnits);
}

private _removeCurrentUnitForType(type: UnitType): void {
this._currentUnits.set(type, null);
this._currentUnits$.next(this._currentUnits);
}

private readonly _unitAdded$ = new Subject<UnitModel>();
Expand All @@ -142,21 +149,28 @@ export class UniverInstanceService extends Disposable implements IUniverInstance
return this._unitAdded$.pipe(filter((unit) => unit.type === type)) as Observable<T>;
}

/**
* Add a unit into Univer.
*
* @ignore
*
* @param unit The unit to be added.
*/
__addUnit(unit: UnitModel): void {
const type = unit.type;

if (!this._unitsByType.has(type)) {
this._unitsByType.set(type, []);
}

this._unitsByType.get(type)!.push(unit);

this._currentUnits$.next({ ...this._currentUnits$.getValue(), [type]: unit });
this._unitAdded$.next(unit);

this.setCurrentUnitForType(unit.getUnitId());
}

private _unitDisposed$ = new Subject<UnitModel>();
unitDisposed$ = this._unitDisposed$.asObservable();
readonly unitDisposed$ = this._unitDisposed$.asObservable();
getTypeOfUnitDisposed$<T extends UnitModel<object, number>>(type: UniverInstanceType): Observable<T> {
return this.unitDisposed$.pipe(filter((unit) => unit.type === type)) as Observable<T>;
}
Expand Down Expand Up @@ -242,10 +256,13 @@ export class UniverInstanceService extends Disposable implements IUniverInstance
const index = units.indexOf(unit);
units.splice(index, 1);

this._unitDisposed$.next(unit);
this._currentUnits$.next({ ...this._currentUnits$.getValue(), [type]: null });
// Firstly un-mark the unit as "current".
this._removeCurrentUnitForType(type);
this._focused$.next(null);

// Then dispose the unit.
this._unitDisposed$.next(unit);

return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ export class PermissionService extends Disposable implements IPermissionService
private _permissionPointMap: Map<string, BehaviorSubject<IPermissionPoint<unknown>>> = new Map();

private _permissionPointUpdate$ = new Subject<IPermissionPoint<unknown>>();

public permissionPointUpdate$ = this._permissionPointUpdate$.asObservable();

deletePermissionPoint(permissionId: string) {
Expand Down
3 changes: 2 additions & 1 deletion packages/facade/src/apis/__tests__/create-test-bed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
} from '@univerjs/core';
import { FormulaDataModel, FunctionService, IFunctionService, LexerTreeBuilder } from '@univerjs/engine-formula';
import { ISocketService, WebSocketService } from '@univerjs/network';
import { SelectionManagerService, SheetInterceptorService, WorkbookPermissionService, WorksheetPermissionService, WorksheetProtectionPointModel, WorksheetProtectionRuleModel } from '@univerjs/sheets';
import { RangeProtectionRuleModel, SelectionManagerService, SheetInterceptorService, WorkbookPermissionService, WorksheetPermissionService, WorksheetProtectionPointModel, WorksheetProtectionRuleModel } from '@univerjs/sheets';
import {
DescriptionService,
FormulaCustomFunctionService,
Expand Down Expand Up @@ -151,6 +151,7 @@ export function createFacadeTestBed(workbookData?: IWorkbookData, dependencies?:
injector.add([WorksheetPermissionService]);
injector.add([WorkbookPermissionService]);
injector.add([WorksheetProtectionPointModel]);
injector.add([RangeProtectionRuleModel]);
injector.add([IAuthzIoService, { useClass: AuthzIoLocalService }]);
injector.add([WorksheetProtectionRuleModel]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import type { IWorkbookData } from '@univerjs/core';
import { AuthzIoLocalService, IAuthzIoService, ILogService, IUniverInstanceService, LocaleType, LogLevel, Plugin, Univer, UniverInstanceType } from '@univerjs/core';
import { LexerTreeBuilder } from '@univerjs/engine-formula';
import { SelectionManagerService, SheetInterceptorService, WorkbookPermissionService, WorksheetPermissionService, WorksheetProtectionPointModel, WorksheetProtectionRuleModel } from '@univerjs/sheets';
import { RangeProtectionRuleModel, SelectionManagerService, SheetInterceptorService, WorkbookPermissionService, WorksheetPermissionService, WorksheetProtectionPointModel, WorksheetProtectionRuleModel } from '@univerjs/sheets';
import type { Dependency } from '@wendellhu/redi';
import { Inject, Injector } from '@wendellhu/redi';
import { EditorService, IEditorService } from '@univerjs/ui';
Expand Down Expand Up @@ -80,6 +80,7 @@ export function createCommandTestBed(workbookData?: IWorkbookData, dependencies?
injector.add([WorksheetPermissionService]);
injector.add([WorksheetProtectionPointModel]);
injector.add([WorkbookPermissionService]);
injector.add([RangeProtectionRuleModel]);
injector.add([IAuthzIoService, { useClass: AuthzIoLocalService }]);
injector.add([WorksheetProtectionRuleModel]);

Expand Down
21 changes: 17 additions & 4 deletions packages/sheets-formula/src/controllers/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,13 @@
* limitations under the License.
*/

import { UniverInstanceType } from '@univerjs/core';
import type { Workbook } from '@univerjs/core';
import { IUniverInstanceService, UniverInstanceType } from '@univerjs/core';
import { getCurrentRangeDisable$, PASTE_SPECIAL_MENU_ID } from '@univerjs/sheets-ui';
import type { IMenuItem } from '@univerjs/ui';
import { getMenuHiddenObservable, IClipboardInterfaceService, MenuGroup, MenuItemType, MenuPosition } from '@univerjs/ui';
import type { IAccessor } from '@wendellhu/redi';
import { combineLatestWith, map, Observable } from 'rxjs';
import { combineLatestWith, map, Observable, of, switchMap } from 'rxjs';

import { RangeProtectionPermissionEditPoint, WorkbookEditablePermission, WorksheetEditPermission, WorksheetSetCellValuePermission } from '@univerjs/sheets';
import { SheetOnlyPasteFormulaCommand } from '../commands/commands/formula-clipboard.command';
Expand Down Expand Up @@ -76,8 +77,20 @@ export function MoreFunctionsMenuItemFactory(accessor: IAccessor): IMenuItem {
}

function menuClipboardDisabledObservable(injector: IAccessor): Observable<boolean> {
const clipboardDisabled$: Observable<boolean> = new Observable((subscriber) => subscriber.next(!injector.get(IClipboardInterfaceService).supportClipboard));
return clipboardDisabled$;
const univerInstanceService = injector.get(IUniverInstanceService);
const workbook$ = univerInstanceService.getCurrentTypeOfUnit$<Workbook>(UniverInstanceType.UNIVER_SHEET);
return workbook$.pipe(
switchMap((workbook) => {
if (!workbook) {
return of(true);
}
const clipboardInterfaceService = injector.get(IClipboardInterfaceService);
if (clipboardInterfaceService) {
return new Observable((subscriber) => subscriber.next(!injector.get(IClipboardInterfaceService).supportClipboard)) as Observable<boolean>;
}
return of(true);
})
);
}

export function PasteFormulaMenuItemFactory(accessor: IAccessor): IMenuItem {
Expand Down
2 changes: 1 addition & 1 deletion packages/sheets-thread-comment/src/types/const.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@

export const SHEETS_THREAD_COMMENT_MODAL = 'univer.sheet.thread-comment-modal';
export const COMMENT_SINGLE_ICON = 'comment-single';
export const SHEETS_THREAD_COMMENT = 'univer.sheet.thread-comment';
export const SHEETS_THREAD_COMMENT = 'SHEET_THREAD_COMMENT';
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,10 @@ describe('test menu items', () => {
disposableCollection.add(menuItem.activated$!.subscribe((v: boolean) => (activated = v)));
disposableCollection.add(menuItem.disabled$!.subscribe((v: boolean) => (disabled = v)));
expect(activated).toBeFalsy();
expect(disabled).toBeFalsy();

select({ startRow: 0, startColumn: 0, endRow: 0, endColumn: 0 });
expect(await commandService.executeCommand(SetBoldCommand.id)).toBeTruthy();
expect(activated).toBe(true);
expect(disabled).toBeFalsy();
});
});
Loading
Loading