chore: prepare for formula refactor

dream-num · Nov 14, 2024 · f0f3af9 · f0f3af9
1 parent 820653c
commit f0f3af9
Show file tree

Hide file tree

Showing 7 changed files with 88 additions and 58 deletions.
diff --git a/packages/engine-formula/docs/TODO.md b/packages/engine-formula/docs/TODO.md
@@ -0,0 +1,7 @@
+# TODOs
+
+- [ ] We should document core modules to make the code more understandable for new contributors.
+- [ ] Remove properties and methods that are not used or not necessary.
+- [ ] Using mutations as remote calling is an anti-pattern and may cause performance issues. We should migrate to a direct way.
+- [ ] Some dependencies are not necessary to be abstract, e.g. `IFormulaService`. Refactoring this can make the code easier to read.
+- [ ] There is no retry or fallback mechanism for web worker failure.
diff --git a/packages/engine-formula/src/plugin.ts b/packages/engine-formula/src/plugin.ts
@@ -24,7 +24,7 @@ import { SetDefinedNameController } from './controller/set-defined-name.controll
 import { SetDependencyController } from './controller/set-dependency.controller';
 import { SetFeatureCalculationController } from './controller/set-feature-calculation.controller';
 import { SetOtherFormulaController } from './controller/set-other-formula.controller';
-import { SetSuperTableController } from './controller/set-super-table.controller';
+// import { SetSuperTableController } from './controller/set-super-table.controller';
 import { Lexer } from './engine/analysis/lexer';
 import { LexerTreeBuilder } from './engine/analysis/lexer-tree-builder';
 import { AstTreeBuilder } from './engine/analysis/parser';
@@ -53,7 +53,7 @@ import {
 import { FunctionService, IFunctionService } from './services/function.service';
 import { IOtherFormulaManagerService, OtherFormulaManagerService } from './services/other-formula-manager.service';
 import { FormulaRuntimeService, IFormulaRuntimeService } from './services/runtime.service';
-import { ISuperTableService, SuperTableService } from './services/super-table.service';
+// import { ISuperTableService, SuperTableService } from './services/super-table.service';
 
 const PLUGIN_NAME = 'UNIVER_ENGINE_FORMULA_PLUGIN';
 
@@ -81,17 +81,12 @@ export class UniverFormulaEnginePlugin extends Plugin {
         touchDependencies(this._injector, [
             [FormulaController],
             [SetDefinedNameController],
-            [SetSuperTableController],
+            // [SetSuperTableController],
+            [SetOtherFormulaController],
+            [SetFeatureCalculationController],
+            [SetDependencyController],
+            [CalculateController],
         ]);
-
-        if (!this._config?.notExecuteFormula) {
-            touchDependencies(this._injector, [
-                [SetOtherFormulaController],
-                [SetFeatureCalculationController],
-                [SetDependencyController],
-                [CalculateController],
-            ]);
-        }
     }
 
     override onRendered(): void {
@@ -106,22 +101,17 @@ export class UniverFormulaEnginePlugin extends Plugin {
     private _initialize() {
         // worker and main thread
         const dependencies: Dependency[] = [
-            // Services
             [IFunctionService, { useClass: FunctionService }],
             [IDefinedNamesService, { useClass: DefinedNamesService }],
             [IActiveDirtyManagerService, { useClass: ActiveDirtyManagerService }],
-            [ISuperTableService, { useClass: SuperTableService }],
+            // [ISuperTableService, { useClass: SuperTableService }],
 
-            // Models
             [FormulaDataModel],
-
-            // Engine
             [LexerTreeBuilder],
 
-            //Controllers
             [FormulaController],
             [SetDefinedNameController],
-            [SetSuperTableController],
+            // [SetSuperTableController],
         ];
 
         if (!this._config?.notExecuteFormula) {

diff --git a/packages/engine-formula/src/services/active-dirty-manager.service.ts b/packages/engine-formula/src/services/active-dirty-manager.service.ts
@@ -14,10 +14,11 @@
  * limitations under the License.
  */
 
-import type { ICommandInfo, IUnitRange, Nullable } from '@univerjs/core';
+import type { ICommandInfo, IDisposable, IUnitRange, Nullable } from '@univerjs/core';
 import type { IDirtyUnitFeatureMap, IDirtyUnitOtherFormulaMap, IDirtyUnitSheetDefinedNameMap, IDirtyUnitSheetNameMap } from '../basics/common';
 import { createIdentifier, Disposable } from '@univerjs/core';
 
+// TODO: this interface deserver better naming.
 export interface IDirtyConversionManagerParams {
     commandId: string;
     getDirtyData: (command: ICommandInfo) => {
@@ -30,17 +31,28 @@ export interface IDirtyConversionManagerParams {
     };
 }
 
-export interface IActiveDirtyManagerService {
-    dispose(): void;
-
+/**
+ * This service is to calculate the dirty area based on the {@link CommandType.MUTATION} executed. Features can
+ * register callback functions to calculate the dirty area.
+ * @deprecated This service is unnecessary to be abstract.
+ */
+export const IActiveDirtyManagerService = createIdentifier<ActiveDirtyManagerService>('univer.formula.active-dirty-manager.service');
+/**
+ * This service is to calculate the dirty area based on the {@link CommandType.MUTATION} executed. Features can
+ * register callback functions to calculate the dirty area.
+ * @deprecated This service is unnecessary to be abstract.
+ */
+export interface IActiveDirtyManagerService extends IDisposable {
+    /** @deprecated `register` should return an observable. */
     remove(commandId: string): void;
-
+    /** @deprecated No one need to check if a feature id exists. */
     get(commandId: string): Nullable<IDirtyConversionManagerParams>;
-
+    /** @deprecated No one need to check if a feature id exists. */
     has(featureId: string): boolean;
 
     register(featureId: string, dirtyConversion: IDirtyConversionManagerParams): void;
 
+    // TODO: should not expose internal implementation. A `onMutationExecuted` method would be much better.
     getDirtyConversionMap(): Map<string, IDirtyConversionManagerParams>;
 }
 
@@ -76,6 +88,3 @@ export class ActiveDirtyManagerService extends Disposable implements IActiveDirt
     }
 }
 
-export const IActiveDirtyManagerService = createIdentifier<ActiveDirtyManagerService>(
-    'univer.formula.active-dirty-manager.service'
-);
diff --git a/packages/engine-formula/src/services/defined-names.service.ts b/packages/engine-formula/src/services/defined-names.service.ts
@@ -20,16 +20,19 @@ import { createIdentifier, Disposable, IUniverInstanceService } from '@univerjs/
 import { Subject } from 'rxjs';
 import { handleRefStringInfo, serializeRange } from '../engine/utils/reference';
 
+// TODO: This deserve a better name, e.g. IDefinedNameItem.
 export interface IDefinedNamesServiceParam {
     id: string;
     name: string;
     formulaOrRefString: string;
     comment?: string;
     localSheetId?: string;
     hidden?: boolean;
-
 }
 
+/**
+ * @deprecated This interface should not co-exist with {@link IDefinedNamesServiceParam}.
+ */
 export interface IDefinedNamesServiceFocusParam extends IDefinedNamesServiceParam {
     unitId: string;
 }
@@ -42,39 +45,52 @@ export interface IDefinedNameMapItem {
     [id: string]: IDefinedNamesServiceParam;
 }
 
+/**
+ * This service is for registering and obtaining defined names.
+ * @deprecated This dependency is not necessary to be abstract.
+ */
+export const IDefinedNamesService = createIdentifier<DefinedNamesService>('engine-formula.defined-names.service');
+/**
+ * This service is for registering and obtaining defined names.
+ * @deprecated This dependency is not necessary to be abstract.
+ */
 export interface IDefinedNamesService {
+    /** @deprecated This method should not co-exist with {@link registerDefinedNames} */
     registerDefinedName(unitId: string, param: IDefinedNamesServiceParam): void;
-
     registerDefinedNames(unitId: string, params: IDefinedNameMapItem): void;
 
     getDefinedNameMap(unitId: string): Nullable<IDefinedNameMapItem>;
 
     getValueByName(unitId: string, name: string): Nullable<IDefinedNamesServiceParam>;
-
     getValueById(unitId: string, id: string): Nullable<IDefinedNamesServiceParam>;
 
+    /** @deprecated This method should not co-exist with {@link removeUnitDefinedName}. */
     removeDefinedName(unitId: string, name: string): void;
-
     removeUnitDefinedName(unitId: string): void;
 
     hasDefinedName(unitId: string): boolean;
 
     setCurrentRange(range: IUnitRange): void;
 
+    /** @deprecated This method is not used and can be used. */
     getCurrentRange(): IUnitRange;
-
+    /** @deprecated This method is not necessary because the caller can directly subscribe currentRange$. */
     getCurrentRangeForString(): string;
 
     currentRange$: Observable<IUnitRange>;
 
+    /** @deprecated Defined names map should be observable instead of using a signal. */
     update$: Observable<unknown>;
 
+    /**
+     * @deprecated This is redundant if we focus a range by using an operation. And this is not a property that lives in
+     * the engine-formula package but the sheet package.
+     */
     focusRange$: Observable<IDefinedNamesServiceFocusParam>;
-
+    /** @deprecated */
     focusRange(unitId: string, id: string): void;
 
     getWorksheetByRef(unitId: string, ref: string): Nullable<Worksheet>;
-
 }
 
 export class DefinedNamesService extends Disposable implements IDefinedNamesService {
@@ -85,7 +101,9 @@ export class DefinedNamesService extends Disposable implements IDefinedNamesServ
     readonly update$ = this._update$.asObservable();
 
     private _currentRange: IUnitRange = {
-        unitId: '', sheetId: '', range: {
+        unitId: '',
+        sheetId: '',
+        range: {
             startRow: 0,
             endRow: 0,
             startColumn: 0,
@@ -113,6 +131,8 @@ export class DefinedNamesService extends Disposable implements IDefinedNamesServ
         return this._univerInstanceService.getUnit<Workbook>(unitId)?.getSheetBySheetName(sheetName);
     }
 
+    // TODO: this focus behavior should be extracted to an operation. The current approach violates our architecture.
+    /** @deprecated */
     focusRange(unitId: string, id: string) {
         const item = this.getValueById(unitId, id);
         if (item == null) {
@@ -192,4 +212,3 @@ export class DefinedNamesService extends Disposable implements IDefinedNamesServ
     }
 }
 
-export const IDefinedNamesService = createIdentifier<DefinedNamesService>('univer.formula.defined-names.service');
diff --git a/packages/engine-formula/src/services/function.service.ts b/packages/engine-formula/src/services/function.service.ts
@@ -15,17 +15,28 @@
  */
 
 import type { IDisposable, Nullable } from '@univerjs/core';
-import { createIdentifier, Disposable, toDisposable } from '@univerjs/core';
-
 import type { IFunctionInfo, IFunctionNames } from '../basics/function';
 import type { BaseFunction } from '../functions/base-function';
+import { createIdentifier, Disposable, toDisposable } from '@univerjs/core';
 
+// TODO: make this dependency not abstract.
+
+/**
+ * This service is for registering and obtaining function executors and descriptions.
+ * @deprecated This dependency is not necessary to be abstract.
+ */
+export const IFunctionService = createIdentifier<FunctionService>('engine-formula.formula-function.service');
+/**
+ * This service is for registering and obtaining function executors and descriptions.
+ * @deprecated This dependency is not necessary to be abstract.
+ */
 export interface IFunctionService {
-    /**
-     * Use register to register a function, new CustomFunction(inject, name)
-     */
+    /* Register function executors. */
     registerExecutors(...functions: BaseFunction[]): void;
+    // TODO: This method maybe not useful because it is unlikely that users will unregister functions.
+    unregisterExecutors(...functionTokens: IFunctionNames[]): void;
 
+    /** @deprecated This function is not used and we shall not expose internal properties. */
     getExecutors(): Map<IFunctionNames, BaseFunction>;
 
     /**
@@ -35,26 +46,19 @@ export interface IFunctionService {
      * You can obtain the calculation result by using
      * const sum = formulaService.getExecutor(FUNCTION_NAMES_MATH.SUM);
      * sum.calculate(new RangeReferenceObject(range, sheetId, unitId), ref2, re3).
-     * @param functionName Function name, which can be obtained through the FUNCTION_NAMES enumeration.
+     * @param functionToken Function name, which can be obtained through the FUNCTION_NAMES enumeration.
      * @returns
      */
     getExecutor(functionToken: IFunctionNames): Nullable<BaseFunction>;
-
     hasExecutor(functionToken: IFunctionNames): boolean;
 
-    unregisterExecutors(...functionTokens: IFunctionNames[]): void;
-
     registerDescriptions(...functions: IFunctionInfo[]): IDisposable;
-
+    // TODO: This method maybe not useful because it is unlikely that users will unregister functions.
+    unregisterDescriptions(...functionTokens: IFunctionNames[]): void;
     getDescriptions(): Map<IFunctionNames, IFunctionInfo>;
-
     getDescription(functionToken: IFunctionNames): Nullable<IFunctionInfo>;
-
     hasDescription(functionToken: IFunctionNames): boolean;
-
-    unregisterDescriptions(...functionTokens: IFunctionNames[]): void;
 }
-export const IFunctionService = createIdentifier<FunctionService>('univer.formula-function.service');
 
 export class FunctionService extends Disposable implements IFunctionService {
     private _functionExecutors: Map<IFunctionNames, BaseFunction> = new Map();

diff --git a/packages/network/src/services/http/http.service.ts b/packages/network/src/services/http/http.service.ts
@@ -15,19 +15,19 @@
  */
 
 import type { IDisposable, Nullable } from '@univerjs/core';
-import { Disposable, remove, toDisposable } from '@univerjs/core';
 import type { Observable } from 'rxjs';
+import type { HTTPResponseType } from './http';
+import type { HTTPHandlerFn, HTTPInterceptorFn, RequestPipe } from './interceptor';
+import type { HTTPRequestMethod } from './request';
+
+import type { HTTPEvent } from './response';
+import { Disposable, remove, toDisposable } from '@univerjs/core';
 import { firstValueFrom, of } from 'rxjs';
 import { concatMap } from 'rxjs/operators';
-
 import { HTTPHeaders } from './headers';
-import type { HTTPResponseType } from './http';
 import { IHTTPImplementation } from './implementations/implementation';
 import { HTTPParams } from './params';
-import type { HTTPRequestMethod } from './request';
 import { HTTPRequest } from './request';
-import type { HTTPEvent } from './response';
-import type { HTTPHandlerFn, HTTPInterceptorFn, RequestPipe } from './interceptor';
 
 export interface IRequestParams {
     /** Query params. These params would be append to the url before the request is sent. */

diff --git a/packages/sheets-ui/src/controllers/defined-name/defined-name.controller.ts b/packages/sheets-ui/src/controllers/defined-name/defined-name.controller.ts
@@ -65,6 +65,7 @@ export class SheetsDefinedNameController extends Disposable {
 
             this._selectionManagerService.setSelections(selections);
 
+            // TODO: we should directly call the operation to update selection to avoid code duplications.
             this._cmdSrv.executeCommand(ScrollToCellOperation.id, selections[0].range);
         }));
     }
-Original file line number
+Diff line change
@@ Expand Up / @@ -65,6 +65,7 @@ export class SheetsDefinedNameController extends Disposable { @@
                 this._selectionManagerService.setSelections(selections);
+                // TODO: we should directly call the operation to update selection to avoid code duplications.
                 this._cmdSrv.executeCommand(ScrollToCellOperation.id, selections[0].range);
             }));
         }
@@ Expand Down @@