scoped Named Expressions bugfix (#647)

* test

* bugfix for namedexpressions

* doc

* linter

* Pu/issue638 (#649)

* scope for listing named expressions

* minor fix

* doc

CHANGELOG.md

@@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 - Added two new fired events, for suspending and resuming execution. (#637)
+- Added listing in scopes to `listNamedExpressions` method. (#638)
+
+### Fixed
+- Fixed issues with scoped named expression. (#646, #641)
 
 ### Fixed
 - Fixed an issue with losing formating info about DateTime numbers. (#626)

src/CrudOperations.ts

@@ -220,8 +220,8 @@ export class CrudOperations {
     const sheetId = this.sheetMapping.fetch(sheetName)
     const originalName = this.sheetMapping.fetchDisplayName(sheetId)
     const oldSheetContent = this.operations.getSheetClipboardCells(sheetId)
-    const version = this.operations.removeSheet(sheetName)
-    this.undoRedo.saveOperation(new RemoveSheetUndoEntry(originalName, sheetId, oldSheetContent, version))
+    const {version, scopedNamedExpressions} = this.operations.removeSheet(sheetName)
+    this.undoRedo.saveOperation(new RemoveSheetUndoEntry(originalName, sheetId, oldSheetContent, scopedNamedExpressions, version))
   }
 
   public renameSheet(sheetId: number, newName: string): Maybe<string> {
@@ -658,7 +658,7 @@ export class CrudOperations {
     }
   }
 
-  public scopeId(sheetName: string | undefined): number | undefined {
+  public scopeId(sheetName?: string): Maybe<number> {
     if (sheetName !== undefined) {
       this.ensureSheetExists(sheetName)
       return this.sheetMapping.fetch(sheetName)

src/HyperFormula.ts

@@ -3407,7 +3407,7 @@ export class HyperFormula implements TypedEmitter {
    * Note that this method may trigger dependency graph recalculation.
    *
    * @param {string} expressionName - expression name, case insensitive.
-   * @param {string?} sheetScope - scope definition, `sheetName` for local scope or `undefined` for global scope
+   * @param {string?} scope - scope definition, `sheetName` for local scope or `undefined` for global scope
    *
    * @fires [[namedExpressionRemoved]] after the expression was removed
    * @fires [[valuesUpdated]] if recalculation was triggered by this change
@@ -3443,27 +3443,37 @@ export class HyperFormula implements TypedEmitter {
 
   /**
    * Lists all named expressions.
-   * Returns an array of expression names as strings
+   * Returns an array of expression names defined in a scope, as strings.
+   *
+   * @param {string?} scope - scope definition, `sheetName` for local scope or `undefined` for global scope
+   *
+   * @throws [[NoSheetWithNameError]] when the given sheet name does not exists
    *
    * @example
    * ```js
    * const hfInstance = HyperFormula.buildFromArray([
    *  ['42'],
    *  ['50'],
+   *  ['60'],
    * ]);
    *
-   * // add two named expressions
+   * // add two named expressions and one scoped
    * hfInstance.addNamedExpression('prettyName', '=Sheet1!A1+100');
    * hfInstance.addNamedExpression('prettyName2', '=Sheet1!A2+100');
+   * hfInstance.addNamedExpression('prettyName3', '=Sheet1!A3+100', 'Sheet1');
    *
    * // list the expressions, should return: ['prettyName', 'prettyName2'] for this example
    * const listOfExpressions = hfInstance.listNamedExpressions();
+   *
+   *  // list the expressions, should return: ['prettyName3'] for this example
+   * const listOfExpressions = hfInstance.listNamedExpressions('Sheet1');
    * ```
    *
    * @category Named Expressions
    */
-  public listNamedExpressions(): string[] {
-    return this._namedExpressions.getAllNamedExpressionsNames()
+  public listNamedExpressions(scope?: string): string[] {
+    const scopeId = this._crudOperations.scopeId(scope)
+    return this._namedExpressions.getAllNamedExpressionsNamesInScope(scopeId)
   }
 
   /**

src/NamedExpressions.ts

@@ -132,7 +132,7 @@ export class NamedExpressions {
     if (sheetId === undefined) {
       return this.workbookStore.isNameAvailable(expressionName)
     } else {
-      return this.worksheetStore(sheetId).isNameAvailable(expressionName)
+      return this.worksheetStore(sheetId)?.isNameAvailable(expressionName) ?? true
     }
   }
 
@@ -149,16 +149,16 @@ export class NamedExpressions {
     if (sheetId === undefined) {
       return this.workbookStore.getExisting(expressionName)
     } else {
-      return this.worksheetStore(sheetId).get(expressionName)
+      return this.worksheetStore(sheetId)?.get(expressionName)
     }
   }
 
   public nearestNamedExpression(expressionName: string, sheetId: number): Maybe<InternalNamedExpression> {
-    return this.worksheetStore(sheetId).get(expressionName) ?? this.workbookStore.getExisting(expressionName)
+    return this.worksheetStore(sheetId)?.get(expressionName) ?? this.workbookStore.getExisting(expressionName)
   }
 
   public isExpressionInScope(expressionName: string, sheetId: number): boolean {
-    return this.worksheetStore(sheetId).has(expressionName)
+    return this.worksheetStore(sheetId)?.has(expressionName) ?? false
   }
 
   public isNameValid(expressionName: string): boolean {
@@ -169,28 +169,29 @@ export class NamedExpressions {
   }
 
   public addNamedExpression(expressionName: string, sheetId?: number, options?: NamedExpressionOptions): InternalNamedExpression {
-    if (sheetId === undefined) {
-      let namedExpression = this.workbookStore.get(expressionName)
-      if (namedExpression) {
-        namedExpression.added = true
-        namedExpression.displayName = expressionName
-        namedExpression.options = options
-      } else {
-        namedExpression = new InternalNamedExpression(expressionName, this.nextAddress(), true, options)
-        this.workbookStore.add(namedExpression)
-      }
-      this.addressCache.set(namedExpression.address.row, namedExpression)
-      return namedExpression
+    const store = sheetId === undefined ? this.workbookStore : this.worksheetStoreOrCreate(sheetId)
+    let namedExpression = store.get(expressionName)
+    if (namedExpression) {
+      namedExpression.added = true
+      namedExpression.displayName = expressionName
+      namedExpression.options = options
     } else {
-      const store = this.worksheetStore(sheetId)
-      const namedExpression = new InternalNamedExpression(expressionName, this.nextAddress(), true, options)
+      namedExpression = new InternalNamedExpression(expressionName, this.nextAddress(), true, options)
       store.add(namedExpression)
-      this.addressCache.set(namedExpression.address.row, namedExpression)
-      return namedExpression
     }
+    this.addressCache.set(namedExpression.address.row, namedExpression)
+    return namedExpression
   }
 
-  private worksheetStore(sheetId: number): WorksheetStore {
+  public restoreNamedExpression(namedExpression: InternalNamedExpression, sheetId?: number): InternalNamedExpression {
+    const store = sheetId === undefined ? this.workbookStore : this.worksheetStoreOrCreate(sheetId)
+    namedExpression.added = true
+    store.add(namedExpression)
+    this.addressCache.set(namedExpression.address.row, namedExpression)
+    return namedExpression
+  }
+
+  private worksheetStoreOrCreate(sheetId: number): WorksheetStore {
     let store = this.worksheetStores.get(sheetId)
     if (!store) {
       store = new WorksheetStore()
@@ -199,18 +200,12 @@ export class NamedExpressions {
     return store
   }
 
+  private worksheetStore(sheetId: number): Maybe<WorksheetStore> {
+    return this.worksheetStores.get(sheetId)
+  }
+
   public namedExpressionOrPlaceholder(expressionName: string, sheetId: number): InternalNamedExpression {
-    let namedExpression = this.worksheetStore(sheetId).get(expressionName)
-    if (namedExpression) {
-      return namedExpression
-    } else {
-      namedExpression = this.workbookStore.get(expressionName)
-      if (namedExpression === undefined) {
-        namedExpression = new InternalNamedExpression(expressionName, this.nextAddress(), false)
-        this.workbookStore.add(namedExpression)
-      }
-      return namedExpression
-    }
+    return this.worksheetStoreOrCreate(sheetId).get(expressionName) ?? this.workbookNamedExpressionOrPlaceholder(expressionName)
   }
 
   public workbookNamedExpressionOrPlaceholder(expressionName: string): InternalNamedExpression {
@@ -229,20 +224,27 @@ export class NamedExpressions {
     } else {
       store = this.worksheetStore(sheetId)
     }
-    const namedExpression = store.get(expressionName)
-    if (namedExpression === undefined || !namedExpression.added) {
+    const namedExpression = store?.get(expressionName)
+    if (store === undefined || namedExpression === undefined || !namedExpression.added) {
       throw 'Named expression does not exist'
     }
     store.remove(expressionName)
+    if(store instanceof WorksheetStore && store.mapping.size === 0) {
+      this.worksheetStores.delete(sheetId!)
+    }
     this.addressCache.delete(namedExpression.address.row)
   }
 
+  public getAllNamedExpressionsNamesInScope(sheetId?: number): string[] {
+    return this.getAllNamedExpressions().filter(({scope}) => scope===sheetId).map((ne) => ne.expression.displayName)
+  }
+
   public getAllNamedExpressionsNames(): string[] {
-    return this.workbookStore.getAllNamedExpressions().map((ne) => ne.displayName)
+    return this.getAllNamedExpressions().map((ne) => ne.expression.displayName)
   }
 
-  public getAllNamedExpressions(): { expression: InternalNamedExpression, scope: Maybe<number> }[] {
-    const storedNamedExpressions: { expression: InternalNamedExpression, scope: Maybe<number> }[] = []
+  public getAllNamedExpressions(): { expression: InternalNamedExpression, scope?: number }[] {
+    const storedNamedExpressions: { expression: InternalNamedExpression, scope?: number }[] = []
 
     this.workbookStore.getAllNamedExpressions().forEach(expr => {
       storedNamedExpressions.push({
@@ -263,6 +265,14 @@ export class NamedExpressions {
     return storedNamedExpressions
   }
 
+  public getAllNamedExpressionsForScope(scope?: number): InternalNamedExpression[] {
+    if(scope === undefined) {
+      return this.workbookStore.getAllNamedExpressions()
+    } else {
+      return this.worksheetStores.get(scope)?.getAllNamedExpressions() ?? []
+    }
+  }
+
   private nextAddress() {
     return simpleCellAddress(NamedExpressions.SHEET_FOR_WORKBOOK_EXPRESSIONS, 0, this.nextNamedExpressionRow++)
   }

src/Operations.ts

@@ -216,7 +216,10 @@ export class Operations {
 
     this.sheetMapping.removeSheet(sheetId)
     this.columnSearch.removeSheet(sheetId)
-    return version!
+    const scopedNamedExpressions = this.namedExpressions.getAllNamedExpressionsForScope(sheetId).map(
+      (namedexpression) => this.removeNamedExpression(namedexpression.normalizeExpressionName(), sheetId)
+    )
+    return {version: version!, scopedNamedExpressions}
   }
 
   public clearSheet(sheetId: number) {
@@ -363,9 +366,8 @@ export class Operations {
 
   public restoreNamedExpression(namedExpression: InternalNamedExpression, content: ClipboardCell, sheetId?: number) {
     const expressionName = namedExpression.displayName
-    const options = namedExpression.options
     this.restoreCell(namedExpression.address, content)
-    const restoredNamedExpression = this.namedExpressions.addNamedExpression(expressionName, sheetId, options)
+    const restoredNamedExpression = this.namedExpressions.restoreNamedExpression(namedExpression, sheetId)
     this.adjustNamedExpressionEdges(restoredNamedExpression, expressionName, sheetId)
   }
 

src/UndoRedo.ts

@@ -241,6 +241,7 @@ export class RemoveSheetUndoEntry extends BaseUndoEntry {
     public readonly sheetName: string,
     public readonly sheetId: number,
     public readonly oldSheetContent: ClipboardCell[][],
+    public readonly scopedNamedExpressions: [InternalNamedExpression, ClipboardCell][],
     public readonly version: number,
   ) {
     super()
@@ -596,6 +597,10 @@ export class UndoRedo {
       }
     }
 
+    for(const [namedexpression, content] of operation.scopedNamedExpressions) {
+      this.operations.restoreNamedExpression(namedexpression, content, sheetId)
+    }
+
     this.restoreOldDataFromVersion(operation.version - 1)
   }
 

test/named-expressions.spec.ts

@@ -357,6 +357,18 @@ describe('Named expressions - store manipulation', () => {
     ])
   })
 
+  it('listing scoped named expressions', () => {
+    const engine = HyperFormula.buildFromSheets({sheet1: [], sheet2: []})
+    engine.addNamedExpression('myName.1', '=42', 'sheet1')
+    engine.addNamedExpression('myName.2', '=42', 'sheet2')
+
+    const namedExpressions = engine.listNamedExpressions('sheet2')
+
+    expect(namedExpressions).toEqual([
+      'myName.2',
+    ])
+  })
+
   it('adding named expressions is case insensitive', () => {
     const engine = HyperFormula.buildEmpty()
 

test/scoped-namedExpressions.spec.ts

@@ -0,0 +1,32 @@
+import {HyperFormula} from '../src'
+import {adr} from './testUtils'
+
+describe('scoped named expressions', () => {
+  it('should be removed when sheet is removed', () => {
+    const engine = HyperFormula.buildFromSheets({'Sheet1': [[]], 'Sheet2': [[]]})
+    engine.addNamedExpression('TRUE', true, 'Sheet1')
+    // eslint-disable-next-line @typescript-eslint/ban-ts-ignore
+    // @ts-ignore
+    expect(engine._namedExpressions.getAllNamedExpressionsNames()).toEqual(['TRUE'])
+    engine.removeSheet('Sheet1')
+    // eslint-disable-next-line @typescript-eslint/ban-ts-ignore
+    // @ts-ignore
+    expect(engine._namedExpressions.getAllNamedExpressionsNames()).toEqual([])
+  })
+
+  it('removal should work with undo of sheet', () => {
+    const engine = HyperFormula.buildFromArray([['=TRUE']])
+    engine.addNamedExpression('TRUE', true, 'Sheet1')
+    engine.removeSheet('Sheet1')
+    engine.undo()
+    expect(engine.getCellValue(adr('A1'))).toEqual(true)
+  })
+
+  it('removal should work with undo of named expression', () => {
+    const engine = HyperFormula.buildFromArray([['=TRUE']])
+    engine.addNamedExpression('TRUE', true, 'Sheet1')
+    engine.removeNamedExpression('TRUE', 'Sheet1')
+    engine.undo()
+    expect(engine.getCellValue(adr('A1'))).toEqual(true)
+  })
+})