chore: prepare for formula refactor

packages/engine-formula/docs/TODO.md

@@ -0,0 +1,12 @@
+# 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.
+- [ ] A graph to demonstrate how the modules are connected.
+- [ ] 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.
+
+## Interface should exposed by the engine
+
+- Set formula data and subscribe to calculation results.

packages/engine-formula/src/basics/common.ts

@@ -119,6 +119,7 @@ export interface IFeatureDirtyRangeType {
     [unitId: string]: Nullable<{ [sheetId: string]: IRange[] }>;
 }
 
+/** @deprecated This interface extends nothing and should be removed to improve simplicity. */
 export interface IArrayFormulaUnitCellType extends IRuntimeUnitDataPrimitiveType {}
 
 export interface IFormulaData {
@@ -132,14 +133,10 @@ export interface IOtherFormulaData {
  * @f  formulaString, the text string of the formula.
  * @si The formula ID can be utilized in scenarios such as copy-pasting and drag-filling to convert formulas into references, eliminating the need for recreating the formulaString.
  */
-export interface IFormulaDataItem {
-    f: string; // formulaString
+export interface IFormulaDataItem extends Pick<ICellData, 'f' | 'si'> {
+    f: string; // Required
     x?: number; // Offset from x direction
     y?: number; // Offset from y direction
-    si?: string; // formulaId,
-    // row: number;
-    // column: number;
-    // sheetId: string;
 }
 
 export interface IOtherFormulaDataItem {

packages/engine-formula/src/commands/mutations/set-formula-data.mutation.ts

@@ -36,3 +36,5 @@ export const SetFormulaDataMutation: IMutation<ISetFormulaDataMutationParams> =
         return true;
     },
 };
+
+// TODO: this mutation is handled in two different modules and may cause the code different to reason.

packages/engine-formula/src/models/formula-data.model.ts

@@ -37,11 +37,13 @@ export interface IRangeChange {
     newCell: IRange | null;
 }
 
+// TODO: drop `Data`, rename it to `FormulaModel`.
+/** `FormulaModel` managers formulas and array formulas. */
 export class FormulaDataModel extends Disposable {
-    private _formulaData: IFormulaData = {};
+    private _formulaData: IFormulaData = {}; // TODO: why values are not cached here but formula strings?
+    // Can we just get values from `Workbook`?
 
     private _arrayFormulaRange: IArrayFormulaRangeType = {};
-
     private _arrayFormulaCellData: IArrayFormulaUnitCellType = {};
 
     constructor(
@@ -161,6 +163,7 @@ export class FormulaDataModel extends Disposable {
         return this._formulaData;
     }
 
+    // TODO: Remove this because this is not used in the codebase.
     setFormulaData(value: IFormulaData) {
         this._formulaData = value;
     }
@@ -270,9 +273,9 @@ export class FormulaDataModel extends Disposable {
         }
     }
 
+    // TODO: Why is this method public and called again in unit tests? This should not be public.
     /**
      * Cache all formulas on the snapshot to the formula model
-     * @returns
      */
     initFormulaData() {
         // TODO@Dushusir: load doc/slide formula data
@@ -292,6 +295,8 @@ export class FormulaDataModel extends Disposable {
             const cellMatrix = worksheet.getCellMatrix();
             const sheetId = worksheet.getSheetId();
 
+            // TODO: Other workbook's formula data are handled in packages/sheets-formula/src/controllers/update-formula.controller.ts
+            // Why are they not handled here?
             initSheetFormulaData(this._formulaData, unitId, sheetId, cellMatrix);
         });
     }
@@ -649,6 +654,8 @@ export class FormulaDataModel extends Disposable {
     }
 }
 
+// TODO: this may duplicate with `mergeFormulaData` in `FormulaDataModel`, because we have two different
+// functions to dump data into the model.
 export function initSheetFormulaData(
     formulaData: IFormulaData,
     unitId: string,

packages/engine-formula/src/models/utils/formula-data-util.ts

@@ -24,7 +24,6 @@ export interface IFormulaIdMap {
     c: number;
 }
 
-// eslint-disable-next-line complexity
 export function updateFormulaDataByCellValue(sheetFormulaDataMatrix: ObjectMatrix<Nullable<IFormulaDataItem>>, newSheetFormulaDataMatrix: ObjectMatrix<IFormulaDataItem | null>, formulaIdMap: Map<string, IFormulaIdMap>, deleteFormulaIdMap: Map<string, string | IFormulaIdMap>, r: number, c: number, cell: Nullable<ICellData>) {
     const formulaString = cell?.f || '';
     const formulaId = cell?.si || '';

packages/engine-formula/src/plugin.ts

@@ -24,7 +24,6 @@ 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 { Lexer } from './engine/analysis/lexer';
 import { LexerTreeBuilder } from './engine/analysis/lexer-tree-builder';
 import { AstTreeBuilder } from './engine/analysis/parser';
@@ -53,7 +52,6 @@ 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';
 
 const PLUGIN_NAME = 'UNIVER_ENGINE_FORMULA_PLUGIN';
 
@@ -81,17 +79,11 @@ export class UniverFormulaEnginePlugin extends Plugin {
         touchDependencies(this._injector, [
             [FormulaController],
             [SetDefinedNameController],
-            [SetSuperTableController],
+            [SetOtherFormulaController],
+            [SetFeatureCalculationController],
+            [SetDependencyController],
+            [CalculateController],
         ]);
-
-        if (!this._config?.notExecuteFormula) {
-            touchDependencies(this._injector, [
-                [SetOtherFormulaController],
-                [SetFeatureCalculationController],
-                [SetDependencyController],
-                [CalculateController],
-            ]);
-        }
     }
 
     override onRendered(): void {
@@ -106,22 +98,15 @@ 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 }],
 
-            // Models
             [FormulaDataModel],
-
-            // Engine
             [LexerTreeBuilder],
 
-            //Controllers
             [FormulaController],
             [SetDefinedNameController],
-            [SetSuperTableController],
         ];
 
         if (!this._config?.notExecuteFormula) {

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'
-);

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');

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();