chore: prepare for formula refactor

examples-node/src/locales.ts

@@ -22,6 +22,12 @@ import sheetsruRU from '@univerjs/sheets/locale/ru-RU';
 import sheetsviVN from '@univerjs/sheets/locale/vi-VN';
 import sheetszhCN from '@univerjs/sheets/locale/zh-CN';
 import sheetszhTW from '@univerjs/sheets/locale/zh-TW';
+import sheetsformulaenUS from '@univerjs/sheets-formula/locale/en-US';
+import sheetsformulafaIR from '@univerjs/sheets-formula/locale/fa-IR';
+import sheetsformularuRU from '@univerjs/sheets-formula/locale/ru-RU';
+import sheetsformulaviVN from '@univerjs/sheets-formula/locale/vi-VN';
+import sheetsformulazhCN from '@univerjs/sheets-formula/locale/zh-CN';
+import sheetsformulazhTW from '@univerjs/sheets-formula/locale/zh-TW';
 import sheetssortenUS from '@univerjs/sheets-sort/locale/en-US';
 import sheetssortfaIR from '@univerjs/sheets-sort/locale/fa-IR';
 import sheetssortruRU from '@univerjs/sheets-sort/locale/ru-RU';
@@ -32,30 +38,36 @@ import sheetssortzhTW from '@univerjs/sheets-sort/locale/zh-TW';
 export const enUS = Tools.deepMerge(
     {},
     sheetsenUS,
+    sheetsformulaenUS,
     sheetssortenUS
 );
 export const ruRU = Tools.deepMerge(
     {},
     sheetsruRU,
+    sheetsformularuRU,
     sheetssortruRU
 );
 export const zhCN = Tools.deepMerge(
     {},
     sheetszhCN,
+    sheetsformulazhCN,
     sheetssortzhCN
 );
 export const zhTW = Tools.deepMerge(
     {},
     sheetszhTW,
+    sheetsformulazhTW,
     sheetssortzhTW
 );
 export const viVN = Tools.deepMerge(
     {},
     sheetsviVN,
+    sheetsformulaviVN,
     sheetssortviVN
 );
 export const faIR = Tools.deepMerge(
     {},
     sheetsfaIR,
+    sheetsformulafaIR,
     sheetssortfaIR
 );

package.json

@@ -72,7 +72,8 @@
         "serve": "^14.2.4",
         "tsx": "^4.19.2",
         "turbo": "^2.2.3",
-        "typescript": "^5.6.3"
+        "typescript": "^5.6.3",
+        "vitest": "^2.1.4"
     },
     "lint-staged": {
         "*": "eslint --fix"

packages/engine-formula/docs/TODO.md

@@ -0,0 +1,13 @@
+# 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.
+- [ ] It would be less performant to parse a formula twice.
+
+## 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/controller/calculate.controller.ts

@@ -35,6 +35,11 @@ import { FormulaDataModel } from '../models/formula-data.model';
 import { ICalculateFormulaService } from '../services/calculate-formula.service';
 import { FormulaExecutedStateType, type IAllRuntimeData } from '../services/runtime.service';
 
+/**
+ * Susbcribe to specific mutations and start calculation process.
+ *
+ * @deprecated Anti-pattern and should be removed.
+ */
 export class CalculateController extends Disposable {
     constructor(
         @ICommandService private readonly _commandService: ICommandService,

packages/engine-formula/src/controller/formula.controller.ts

@@ -60,6 +60,9 @@ import { functionWeb } from '../functions/web/function-map';
 import { IFunctionService } from '../services/function.service';
 import { PLUGIN_CONFIG_KEY } from './config.schema';
 
+/**
+ * Formula controller register commands and builtin functions to the engine.
+ */
 export class FormulaController extends Disposable {
     constructor(
         @ICommandService private readonly _commandService: ICommandService,
@@ -78,6 +81,10 @@ export class FormulaController extends Disposable {
     }
 
     private _registerCommands(): void {
+        // TODO: these commands will be removed in hte future because it is an anti-pattern
+        // to use mutations to sync between the main thread and worker thread, or as events.
+
+        // The following actions should be dispatched in a new `IFormulaAPIService` service.
         [
             SetFormulaDataMutation,
             SetArrayFormulaDataMutation,

packages/engine-formula/src/controller/set-defined-name.controller.ts

@@ -24,6 +24,11 @@ import { Disposable, ICommandService } from '@univerjs/core';
 import { RemoveDefinedNameMutation, SetDefinedNameMutation } from '../commands/mutations/set-defined-name.mutation';
 import { IDefinedNamesService } from '../services/defined-names.service';
 
+/**
+ * `SetDefinedNameController` is responsible for listening to the `SetDefinedNameMutation` and `RemoveDefinedNameMutation`
+ * commands to register or remove defined names in both main thread and web worker.
+ * @deprecated Anti-pattern: Do not use command system as rpc calling method.
+ */
 export class SetDefinedNameController extends Disposable {
     constructor(
         @ICommandService private readonly _commandService: ICommandService,

packages/engine-formula/src/controller/set-dependency.controller.ts

@@ -19,7 +19,8 @@ import type { ISetDefinedNameMutationParam } from '../commands/mutations/set-def
 
 import type {
     IRemoveFeatureCalculationMutationParam,
-    ISetFeatureCalculationMutation } from '../commands/mutations/set-feature-calculation.mutation';
+    ISetFeatureCalculationMutation,
+} from '../commands/mutations/set-feature-calculation.mutation';
 import type { ISetFormulaDataMutationParams } from '../commands/mutations/set-formula-data.mutation';
 import type { IRemoveOtherFormulaMutationParams, ISetOtherFormulaMutationParams } from '../commands/mutations/set-other-formula.mutation';
 import { Disposable, ICommandService, ObjectMatrix } from '@univerjs/core';
@@ -33,10 +34,12 @@ import { RemoveOtherFormulaMutation, SetOtherFormulaMutation } from '../commands
 import { IDependencyManagerService } from '../services/dependency-manager.service';
 import { IFeatureCalculationManagerService } from '../services/feature-calculation-manager.service';
 
+/**
+ * @deprecated This controller is used for an anti-pattern to sync between threads and it should be removed.
+ */
 export class SetDependencyController extends Disposable {
     constructor(
         @ICommandService private readonly _commandService: ICommandService,
-        @IFeatureCalculationManagerService
         @IDependencyManagerService private readonly _dependencyManagerService: IDependencyManagerService,
         @IFeatureCalculationManagerService private readonly _featureCalculationManagerService: IFeatureCalculationManagerService) {
         super();

packages/engine-formula/src/controller/set-feature-calculation.controller.ts

@@ -26,6 +26,9 @@ import {
 } from '../commands/mutations/set-feature-calculation.mutation';
 import { IFeatureCalculationManagerService } from '../services/feature-calculation-manager.service';
 
+/**
+ * @deprecated This controller is used for an anti-pattern to sync between threads and it should be removed.
+ */
 export class SetFeatureCalculationController extends Disposable {
     constructor(
         @ICommandService private readonly _commandService: ICommandService,

packages/engine-formula/src/controller/set-other-formula.controller.ts

@@ -20,16 +20,17 @@ import type { IOtherFormulaData } from '../basics/common';
 import type { IRemoveOtherFormulaMutationParams, ISetOtherFormulaMutationParams } from '../commands/mutations/set-other-formula.mutation';
 import { Disposable, ICommandService } from '@univerjs/core';
 import { RemoveOtherFormulaMutation, SetOtherFormulaMutation } from '../commands/mutations/set-other-formula.mutation';
-import { IDependencyManagerService } from '../services/dependency-manager.service';
 import {
     IOtherFormulaManagerService,
 } from '../services/other-formula-manager.service';
 
+/**
+ * @deprecated This controller is used for an anti-pattern to sync between threads and it should be removed.
+ */
 export class SetOtherFormulaController extends Disposable {
     constructor(
         @ICommandService private readonly _commandService: ICommandService,
-        @IOtherFormulaManagerService private readonly _otherFormulaManagerService: IOtherFormulaManagerService,
-        @IDependencyManagerService private readonly _dependencyManagerService: IDependencyManagerService
+        @IOtherFormulaManagerService private readonly _otherFormulaManagerService: IOtherFormulaManagerService
     ) {
         super();
 

packages/engine-formula/src/engine/analysis/lexer-tree-builder.ts

@@ -56,6 +56,9 @@ export const FormulaLexerNodeCache = new FormulaAstLRU<LexerNode>(FORMULA_CACHE_
 
 export const FormulaSequenceNodeCache = new FormulaAstLRU<Array<string | ISequenceNode>>(FORMULA_CACHE_LRU_COUNT);
 
+/**
+ * This class is used to transform a formula string into a lexer tree.
+ */
 export class LexerTreeBuilder extends Disposable {
     private _currentLexerNode: LexerNode = new LexerNode();
 

packages/engine-formula/src/engine/analysis/lexer.ts

@@ -14,14 +14,13 @@
  * limitations under the License.
  */
 
-import { Disposable, Inject } from '@univerjs/core';
-
-import { IDefinedNamesService } from '../../services/defined-names.service';
 import type { ISequenceArray } from '../utils/sequence';
-import { sequenceNodeType } from '../utils/sequence';
+import { Disposable, Inject } from '@univerjs/core';
+import { ErrorType } from '../../basics/error-type';
 import { operatorToken } from '../../basics/token';
 import { IFormulaCurrentConfigService } from '../../services/current-data.service';
-import { ErrorType } from '../../basics/error-type';
+import { IDefinedNamesService } from '../../services/defined-names.service';
+import { sequenceNodeType } from '../utils/sequence';
 import { LexerTreeBuilder } from './lexer-tree-builder';
 
 export class Lexer extends Disposable {

packages/engine-formula/src/engine/ast-node/suffix-node.ts

@@ -107,6 +107,7 @@ export class SuffixNode extends BaseAstNode {
             return ErrorValueObject.create(ErrorType.VALUE);
         }
 
+        // TODO: why we parse it here again?
         const lexerNode = this._lexer.treeBuilder(formulaString);
 
         return ErrorValueObject.create(ErrorType.VALUE);

packages/engine-formula/src/engine/dependency/formula-dependency.ts

@@ -59,15 +59,14 @@ export interface IFormulaDependencyGenerator {
 
 export const IFormulaDependencyGenerator = createIdentifier<IFormulaDependencyGenerator>('engine-formula.dependency-generator');
 
-export class FormulaDependencyGenerator extends Disposable {
+export class FormulaDependencyGenerator extends Disposable implements IFormulaDependencyGenerator {
     private _updateRangeFlattenCache = new Map<string, Map<string, IRange[]>>();
 
     constructor(
         @IFormulaCurrentConfigService protected readonly _currentConfigService: IFormulaCurrentConfigService,
         @IFormulaRuntimeService protected readonly _runtimeService: IFormulaRuntimeService,
         @IOtherFormulaManagerService protected readonly _otherFormulaManagerService: IOtherFormulaManagerService,
-        @IFeatureCalculationManagerService
-        private readonly _featureCalculationManagerService: IFeatureCalculationManagerService,
+        @IFeatureCalculationManagerService private readonly _featureCalculationManagerService: IFeatureCalculationManagerService,
         @Inject(Interpreter) private readonly _interpreter: Interpreter,
         @Inject(AstTreeBuilder) protected readonly _astTreeBuilder: AstTreeBuilder,
         @Inject(Lexer) protected readonly _lexer: Lexer,
@@ -82,7 +81,6 @@ export class FormulaDependencyGenerator extends Disposable {
 
     async generate() {
         this._updateRangeFlatten();
-        // const formulaInterpreter = Interpreter.create(interpreterDatasetConfig);
 
         const formulaData = this._currentConfigService.getFormulaData();
 
@@ -109,28 +107,26 @@ export class FormulaDependencyGenerator extends Disposable {
 
         const unitData = this._currentConfigService.getUnitData();
 
-        const treeList = await this._generateTreeList(formulaData, otherFormulaData, unitData);
-
-        const updateTreeList = this._getUpdateTreeListAndMakeDependency(treeList);
-
-        let finalTreeList = this._calculateRunList(updateTreeList);
-
-        const hasFeatureCalculation = this._dependencyFeatureCalculation(finalTreeList);
+        let sortedFormulaTrees = this._calculateRunList(
+            this._getUpdateTreeListAndMakeDependency(
+                await this._generateTreeList(formulaData, otherFormulaData, unitData
+                )
+            )
+        );
 
+        // TODO: if there are
+        const hasFeatureCalculation = this._dependencyFeatureCalculation(sortedFormulaTrees);
         if (hasFeatureCalculation) {
-            finalTreeList.forEach((tree) => {
-                tree.resetState();
-            });
-            finalTreeList = this._calculateRunList(finalTreeList);
+            sortedFormulaTrees.forEach((tree) => tree.resetState());
+            sortedFormulaTrees = this._calculateRunList(sortedFormulaTrees);
         }
 
-        const isCycleDependency = this._checkIsCycleDependency(finalTreeList);
-
-        if (isCycleDependency) {
+        const hasCycleDependency = this._checkIsCycleDependency(sortedFormulaTrees);
+        if (hasCycleDependency) {
             this._runtimeService.enableCycleDependency();
         }
 
-        return Promise.resolve(finalTreeList);
+        return Promise.resolve(sortedFormulaTrees);
     }
 
     private _dependencyFeatureCalculation(newTreeList: IFormulaDependencyTree[]) {
@@ -590,6 +586,7 @@ export class FormulaDependencyGenerator extends Disposable {
                     if (treeId != null) {
                         FDtree.treeId = treeId;
                     } else {
+                        // TODO: `addFormulaDependencyByDefinedName` should be merged into `addFormulaDependency`.
                         this._dependencyManagerService.addFormulaDependency(unitId, sheetId, row, column, FDtree);
                         this._dependencyManagerService.addFormulaDependencyByDefinedName(FDtree);
                     }
@@ -810,6 +807,8 @@ export class FormulaDependencyGenerator extends Disposable {
         return rangeList;
     }
 
+    // TODO: this should be merged with `_generateTreeList`.
+
     /**
      * Build a formula dependency tree based on the dependency relationships.
      * @param treeList

packages/engine-formula/src/engine/utils/generate-ast-node.ts

@@ -46,6 +46,9 @@ export function generateAstNode(unitId: string, formulaString: string, lexer: Le
     }
 
     // suffix Express, 1+(3*4=4)*5+1 convert to 134*4=5*1++
+    // TODO: why would be parse the formula string twice? First as lexer node and then as ast node?
+    // It may cause performance issues.
+    // And the names are inaccurate.
 
     astNode = astTreeBuilder.parse(lexerNode as LexerNode);
 

packages/engine-formula/src/facade/f-formula.ts

@@ -76,6 +76,7 @@ export class FFormula extends FBase {
     onCalculationEnd(): Promise<void> {
         return new Promise((resolve, reject) => {
             const timer = setTimeout(() => {
+                // You should comment the above line to debug test.
                 reject(new Error('Calculation end timeout'));
             }, 30_000);
 

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

@@ -37,11 +37,15 @@ 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 +165,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 +275,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 +297,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 +656,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,