fix(core): fix disposing univer causing error (#2515)

* fix(core): fix disposing univer causing error

* chore: upgrade redi to 0.15.4

* test: add e2e disposing API

* docs: add guide of e2e tests

* fix: fix playwright config

CONTRIBUTING.md

@@ -142,6 +142,26 @@ With the help of vscode and its rich ecosystem, you could directly debug unit te
 
 ![](./docs/img/debug-unit-test.png)
 
+### E2E test
+
+You may need to install dependencies of Playwright manually by running the following command:
+
+```shell
+pnpm exec playwright install
+```
+
+If you would like to develop E2E tests, you can use the following command to run the dev server:
+
+```shell
+pnpm dev:e2e
+```
+
+and then run the following command to run E2E tests:
+
+```shell
+pnpm test:e2e
+```
+
 ### Clean code
 
 > Programs are meant to be ready by humans and only icidentally for computers to execute. - Harold Abelson

common/storybook/package.json

@@ -30,7 +30,7 @@
  "@univerjs/core": "workspace:*",
  "@univerjs/design": "workspace:*",
  "@univerjs/ui": "workspace:*",
- "@wendellhu/redi": "0.15.2",
+ "@wendellhu/redi": "0.15.4",
  "css-loader": "^6.10.0",
  "less-loader": "^12.2.0",
  "storybook": "8.1.6",

e2e/disposing/disposing.spec.ts

@@ -0,0 +1,52 @@
+/**
+ * Copyright 2023-present DreamNum Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http:https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { expect, test } from '@playwright/test';
+
+// The type definition is copied from:
+// examples/src/plugins/debugger/controllers/e2e/e2e-memory.controller.ts
+export interface IE2EControllerAPI {
+ loadAndRelease(id: number): Promise<void>;
+ loadDefaultSheet(): Promise<void>;
+ disposeUniver(): Promise<void>;
+}
+
+declare global {
+ // eslint-disable-next-line ts/naming-convention
+ interface Window {
+ E2EControllerAPI: IE2EControllerAPI;
+ }
+}
+
+test('no error on constructing and disposing', 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.loadDefaultSheet());
+ await page.waitForTimeout(2000); // wait for long enough to let the GC do its job
+
+ await page.evaluate(() => window.E2EControllerAPI.disposeUniver());
+ await page.waitForTimeout(2000); // wait for long enough to let the GC do its job
+
+ expect(errored).toBeFalsy();
+});

e2e/memory/memory.spec.ts

@@ -21,14 +21,15 @@ import { expect, test } from '@playwright/test';
 
 // The type definition is copied from:
 // examples/src/plugins/debugger/controllers/e2e/e2e-memory.controller.ts
-export interface IE2EMemoryControllerAPI {
+export interface IE2EControllerAPI {
  loadAndRelease(id: number): Promise<void>;
- getHeapMemoryUsage(): number;
+ disposeUniver(): Promise<void>;
 }
+
 declare global {
  // eslint-disable-next-line ts/naming-convention
  interface Window {
- E2EMemoryAPI: IE2EMemoryControllerAPI;
+ E2EControllerAPI: IE2EControllerAPI;
  }
 }
 
@@ -39,12 +40,12 @@ test('memory', async ({ page }) => {
  const memoryBeforeLoad = (await getMetrics(page)).JSHeapUsedSize;
  console.log('Memory before load:', memoryBeforeLoad);
 
- await page.evaluate(() => window.E2EMemoryAPI.loadAndRelease(1));
+ await page.evaluate(() => window.E2EControllerAPI.loadAndRelease(1));
  await page.waitForTimeout(5000); // wait for long enough to let the GC do its job
  const memoryAfterFirstLoad = (await getMetrics(page)).JSHeapUsedSize;
  console.log('Memory after first load:', memoryAfterFirstLoad);
 
- await page.evaluate(() => window.E2EMemoryAPI.loadAndRelease(2));
+ await page.evaluate(() => window.E2EControllerAPI.loadAndRelease(2));
  const memoryAfterSecondLoad = (await getMetrics(page)).JSHeapUsedSize;
  console.log('Memory after second load:', memoryAfterSecondLoad);
 

examples/esbuild.config.mjs

@@ -13,6 +13,7 @@ import vue from 'esbuild-plugin-vue3';
 const nodeModules = path.resolve(process.cwd(), './node_modules');
 
 const args = minimist(process.argv.slice(2));
+const isE2E = !!args.e2e;
 
 // User should also config their bunlder to build monaco editor's resources for web worker.
 const monacoEditorEntryPoints = ['vs/language/typescript/ts.worker.js', 'vs/editor/editor.worker.js'];
@@ -99,7 +100,7 @@ const ctx = await esbuild[args.watch ? 'context' : 'build']({
  'process.env.GIT_COMMIT_HASH': `"${gitCommitHash}"`,
  'process.env.GIT_REF_NAME': `"${gitRefName}"`,
  'process.env.BUILD_TIME': `"${new Date().toISOString()}"`,
- 'process.env.IS_E2E': args.e2e ? 'true' : 'false',
+ 'process.env.IS_E2E': isE2E ? 'true' : 'false',
  },
 });
 
@@ -109,7 +110,7 @@ if (args.watch) {
 
  const { port } = await ctx.serve({
  servedir: './local',
- port: 3002,
+ port: isE2E ? 3000 : 3002,
  });
 
  const url = `http:https://localhost:${port}`;

examples/package.json

@@ -50,7 +50,7 @@
  "@univerjs/thread-comment-ui": "workspace:*",
  "@univerjs/ui": "workspace:*",
  "@univerjs/uniscript": "workspace:*",
- "@wendellhu/redi": "0.15.2",
+ "@wendellhu/redi": "0.15.4",
  "clsx": "^2.1.1",
  "monaco-editor": "0.49.0",
  "react": "18.3.1",

packages/core/package.json

@@ -59,7 +59,7 @@
  "build": "tsc && vite build"
  },
  "peerDependencies": {
- "@wendellhu/redi": "0.15.2",
+ "@wendellhu/redi": "0.15.4",
  "rxjs": ">=7.0.0"
  },
  "dependencies": {
@@ -71,7 +71,7 @@
  "devDependencies": {
  "@types/numeral": "^2.0.5",
  "@univerjs/shared": "workspace:*",
- "@wendellhu/redi": "0.15.2",
+ "@wendellhu/redi": "0.15.4",
  "rxjs": "^7.8.1",
  "typescript": "^5.4.5",
  "vite": "^5.2.13",

packages/core/src/services/permission/permission.service.ts

@@ -81,10 +81,11 @@ export class PermissionService extends Disposable implements IPermissionService
  const subjectList = permissionIdList.map((id) => {
  const subject = this._permissionPointMap?.get(id);
  if (!subject) {
- throw new Error(`${id} permissionPoint is not exist`);
+ throw new Error(`[PermissionService]: ${id} permissionPoint does not exist!`);
  }
  return subject.asObservable();
  });
+
  return combineLatest(subjectList).pipe(
  // Check that all permissions exist
  map((list) => {
@@ -97,10 +98,11 @@ export class PermissionService extends Disposable implements IPermissionService
  const valueList = permissionIdList.map((id) => {
  const subject = this._permissionPointMap?.get(id);
  if (!subject) {
- throw new Error(`${id} permissionPoint is not exist`);
+ throw new Error(`[PermissionService]: ${id} permissionPoint does not exist!`);
  }
  return subject.getValue();
  });
+
  return valueList;
  }
 }

packages/data-validation/package.json

@@ -64,7 +64,7 @@
  "peerDependencies": {
  "@univerjs/core": "workspace:*",
  "@univerjs/sheets": "workspace:*",
- "@wendellhu/redi": "0.15.2",
+ "@wendellhu/redi": "0.15.4",
  "rxjs": ">=7.0.0"
  },
  "dependencies": {
@@ -74,7 +74,7 @@
  "@univerjs/core": "workspace:*",
  "@univerjs/shared": "workspace:*",
  "@univerjs/sheets": "workspace:*",
- "@wendellhu/redi": "0.15.2",
+ "@wendellhu/redi": "0.15.4",
  "rxjs": "^7.8.1",
  "typescript": "^5.4.5",
  "vite": "^5.2.13",

packages/debugger/package.json

@@ -60,7 +60,7 @@
  "@univerjs/sheets": "workspace:*",
  "@univerjs/sheets-drawing-ui": "workspace:*",
  "@univerjs/ui": "workspace:*",
- "@wendellhu/redi": "0.15.2",
+ "@wendellhu/redi": "0.15.4",
  "react": "^16.9.0 || ^17.0.0 || ^18.0.0",
  "rxjs": ">=7.0.0"
  },
@@ -75,7 +75,7 @@
  "@univerjs/sheets-drawing-ui": "workspace:*",
  "@univerjs/ui": "workspace:*",
  "@vitejs/plugin-vue": "^5.0.4",
- "@wendellhu/redi": "0.15.2",
+ "@wendellhu/redi": "0.15.4",
  "rxjs": "^7.8.1",
  "typescript": "^5.4.5",
  "vite": "^5.2.13",

packages/debugger/src/commands/commands/unit.command.ts

@@ -14,7 +14,24 @@
  * limitations under the License.
  */
 
-import { CommandType, type ICommand, IUniverInstanceService, UniverInstanceType } from '@univerjs/core';
+import { CommandType, type ICommand, IUniverInstanceService, type Univer, UniverInstanceType } from '@univerjs/core';
+
+declare global {
+ // eslint-disable-next-line ts/naming-convention
+ interface Window {
+ univer?: Univer;
+ }
+}
+
+export const DisposeUniverCommand: ICommand = {
+ id: 'debugger.command.dispose-unit',
+ type: CommandType.COMMAND,
+ handler: () => {
+ window.univer?.dispose();
+
+ return true;
+ },
+};
 
 export const DisposeCurrentUnitCommand: ICommand = {
  id: 'debugger.command.dispose-current-unit',

packages/debugger/src/controllers/debugger.controller.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { Disposable, ICommandService, IUniverInstanceService } from '@univerjs/core';
+import { Disposable, ICommandService } from '@univerjs/core';
 import type { IMenuItemFactory, MenuConfig } from '@univerjs/ui';
 import { ComponentManager, IMenuService } from '@univerjs/ui';
 import { Inject, Injector } from '@wendellhu/redi';
@@ -34,7 +34,7 @@ import { TEST_EDITOR_CONTAINER_COMPONENT } from '../views/test-editor/component-
 // @ts-ignore
 import VueI18nIcon from '../components/VueI18nIcon.vue';
 
-import { CreateEmptySheetCommand, DisposeCurrentUnitCommand } from '../commands/commands/unit.command';
+import { CreateEmptySheetCommand, DisposeCurrentUnitCommand, DisposeUniverCommand } from '../commands/commands/unit.command';
 import { CreateFloatDomCommand } from '../commands/commands/float-dom.command';
 import { ImageDemo } from '../components/Image';
 import { ChangeUserCommand } from '../commands/operations/change-user.operation';
@@ -45,6 +45,7 @@ import {
  CreateFloatDOMMenuItemFactory,
  DialogMenuItemFactory,
  DisposeCurrentUnitMenuItemFactory,
+ DisposeUniverItemFactory,
  FloatDomMenuItemFactory,
  LocaleMenuItemFactory,
  MessageMenuItemFactory,
@@ -70,12 +71,11 @@ export class DebuggerController extends Disposable {
  @Inject(Injector) private readonly _injector: Injector,
  @IMenuService private readonly _menuService: IMenuService,
  @ICommandService private readonly _commandService: ICommandService,
- @Inject(IUniverInstanceService) private _univerInstanceService: IUniverInstanceService,
  @Inject(ComponentManager) private readonly _componentManager: ComponentManager
  ) {
  super();
- this._initializeContextMenu();
 
+ this._initializeContextMenu();
  this._initCustomComponents();
 
  [
@@ -88,6 +88,7 @@ export class DebuggerController extends Disposable {
  SidebarOperation,
  SetEditable,
  SaveSnapshotOptions,
+ DisposeUniverCommand,
  DisposeCurrentUnitCommand,
  CreateEmptySheetCommand,
  CreateFloatDomCommand,
@@ -112,6 +113,7 @@ export class DebuggerController extends Disposable {
  SetEditableMenuItemFactory,
  SaveSnapshotSetEditableMenuItemFactory,
  UnitMenuItemFactory,
+ DisposeUniverItemFactory,
  DisposeCurrentUnitMenuItemFactory,
  CreateEmptySheetMenuItemFactory,
  FloatDomMenuItemFactory,

packages/debugger/src/controllers/e2e/e2e-memory.controller.ts

@@ -14,7 +14,8 @@
  * limitations under the License.
  */
 
-import { IUniverInstanceService, LifecycleStages, LocaleType, OnLifecycle, UniverInstanceType } from '@univerjs/core';
+import { ICommandService, IUniverInstanceService, LifecycleStages, LocaleType, OnLifecycle, UniverInstanceType } from '@univerjs/core';
+import { DisposeUniverCommand } from '../../commands/commands/unit.command';
 
 const DEFAULT_WORKBOOK_DATA_DEMO = {
  id: 'test',
@@ -62,14 +63,16 @@ const DEFAULT_WORKBOOK_DATA_DEMO = {
 const AWAIT_LOADING_TIMEOUT = 5000;
 const AWAIT_DISPOSING_TIMEOUT = 5000;
 
-export interface IE2EMemoryControllerAPI {
+export interface IE2EControllerAPI {
  loadAndRelease(id: number): Promise<void>;
+ loadDefaultSheet(): Promise<void>;
+ disposeUniver(): Promise<void>;
 }
 
 declare global {
  // eslint-disable-next-line ts/naming-convention
  interface Window {
- E2EMemoryAPI: IE2EMemoryControllerAPI;
+ E2EControllerAPI: IE2EControllerAPI;
  }
 }
 
@@ -79,14 +82,17 @@ declare global {
 @OnLifecycle(LifecycleStages.Starting, E2EMemoryController)
 export class E2EMemoryController {
  constructor(
- @IUniverInstanceService private readonly _univerInstanceService: IUniverInstanceService
+ @IUniverInstanceService private readonly _univerInstanceService: IUniverInstanceService,
+ @ICommandService private readonly _commandService: ICommandService
  ) {
  this._initPlugin();
  }
 
  private _initPlugin(): void {
- window.E2EMemoryAPI = {
+ window.E2EControllerAPI = {
  loadAndRelease: (id) => this._releaseAndLoad(id),
+ loadDefaultSheet: () => this._loadDefaultSheet(),
+ disposeUniver: () => this._disposeUniver(),
  };
  }
 
@@ -97,6 +103,15 @@ export class E2EMemoryController {
  this._univerInstanceService.disposeUnit(unitId);
  await timer(AWAIT_DISPOSING_TIMEOUT);
  }
+
+ private async _loadDefaultSheet(): Promise<void> {
+ this._univerInstanceService.createUnit(UniverInstanceType.UNIVER_SHEET, DEFAULT_WORKBOOK_DATA_DEMO);
+ await timer(AWAIT_LOADING_TIMEOUT);
+ }
+
+ private async _disposeUniver(): Promise<void> {
+ await this._commandService.executeCommand(DisposeUniverCommand.id);
+ }
 }
 
 function timer(timeout: number): Promise<void> {