Test the cleanup of placeholder sheets

Author: sequba

.eslintrc.js

@@ -128,7 +128,7 @@ module.exports = {
     'jest/no-standalone-expect': 'warn',
     'jest/no-test-prefixes': 'off',
     'jest/prefer-to-be': 'warn',
-    'jest/prefer-to-have-length': 'warn',
+    'jest/prefer-to-have-length': 'off',
   },
   overrides: [
     {

src/DependencyGraph/AddressMapping/AddressMapping.ts

@@ -180,9 +180,11 @@ export class AddressMapping {
 
   /**
    * Removes a sheet from the address mapping.
+   * If sheet does not exist, does nothing.
+   * @returns {boolean} true if sheet was removed, false if it did not exist.
    */
-  public removeSheet(sheetId: number): void {
-    this.mapping.delete(sheetId)
+  public removeSheetIfExists(sheetId: number): boolean {
+    return this.mapping.delete(sheetId)
   }
 
   /**

src/DependencyGraph/DependencyGraph.ts

@@ -323,14 +323,14 @@ export class DependencyGraph {
    * - If nothing is left, removes the sheet from sheet mapping and address mapping.
    * - Otherwise, marks it as placeholder.
    */
-  public removeSheet(sheetId: number) {
+  public removeSheet(sheetId: number): void {
     this.clearSheet(sheetId)
     const addressMappingCleared = !this.addressMapping.hasAnyEntries(sheetId)
     const rangeMappingCleared = this.rangeMapping.getNumberOfRangesInSheet(sheetId) === 0
 
     if (addressMappingCleared && rangeMappingCleared) {
-      this.sheetMapping.removeSheet(sheetId)
-      this.addressMapping.removeSheet(sheetId)
+      this.sheetMapping.removeSheetIfExists(sheetId)
+      this.addressMapping.removeSheetIfExists(sheetId)
     } else {
       this.sheetMapping.markSheetAsPlaceholder(sheetId)
     }
@@ -352,7 +352,7 @@ export class DependencyGraph {
 
     this.mergeRangeVertices(sheetToKeep, placeholderSheetToDelete)
     this.mergeCellVertices(sheetToKeep, placeholderSheetToDelete)
-    this.addressMapping.removeSheet(placeholderSheetToDelete)
+    this.addressMapping.removeSheetIfExists(placeholderSheetToDelete)
     this.addStructuralNodesToChangeSet()
   }
 
@@ -1340,7 +1340,7 @@ export class DependencyGraph {
           !this.addressMapping.hasAnyEntries(sheetId) &&
           this.rangeMapping.getNumberOfRangesInSheet(sheetId) === 0) {
         this.sheetMapping.removeSheet(sheetId, { includePlaceholders: true })
-        this.addressMapping.removeSheet(sheetId)
+        this.addressMapping.removeSheetIfExists(sheetId)
       }
     }
   }

src/DependencyGraph/SheetMapping.ts

@@ -166,7 +166,7 @@ export class SheetMapping {
 
     this.lastSheetId++
     const sheet = new Sheet(this.lastSheetId, newSheetDisplayName)
-    this.storeSheetInMappings(sheet)
+    this._storeSheetInMappings(sheet)
     return sheet.id
   }
 
@@ -197,7 +197,7 @@ export class SheetMapping {
     }
 
     const sheet = new Sheet(sheetId, sheetDisplayName)
-    this.storeSheetInMappings(sheet)
+    this._storeSheetInMappings(sheet)
   }
 
   /**
@@ -212,7 +212,7 @@ export class SheetMapping {
 
     this.lastSheetId++
     const sheet = new Sheet(this.lastSheetId, sheetName, true)
-    this.storeSheetInMappings(sheet)
+    this._storeSheetInMappings(sheet)
     return sheet.id
   }
 
@@ -237,17 +237,44 @@ export class SheetMapping {
       this.lastSheetId = sheetId
     }
     const sheet = new Sheet(sheetId, sheetDisplayName, true)
-    this.storeSheetInMappings(sheet)
+    this._storeSheetInMappings(sheet)
   }
 
   /**
-   * Removes sheet with given ID. Ignores placeholders
+   * Removes sheet with given ID.
    * @throws {NoSheetWithIdError} if the sheet with the given ID does not exist or is a placeholder
    */
   public removeSheet(sheetId: number, options: SheetMappingQueryOptions = {}): void {
     const sheet = this._getSheetOrThrowError(sheetId, options)
     this.allSheets.delete(sheetId)
     this.mappingFromCanonicalNameToId.delete(sheet.canonicalName)
+
+    if (sheetId === this.lastSheetId) {
+      this.lastSheetId--
+    }
+  }
+
+  /**
+   *
+   * Removes sheet with given ID.
+   * If sheet does not exist, does nothing.
+   * @returns {boolean} true if sheet was removed, false if it did not exist.
+   */
+  public removeSheetIfExists(sheetId: number, options: SheetMappingQueryOptions = {}): boolean {
+    const sheet = this._getSheet(sheetId, options)
+
+    if (!sheet) {
+      return false
+    }
+
+    this.allSheets.delete(sheetId)
+    this.mappingFromCanonicalNameToId.delete(sheet.canonicalName)
+
+    if (sheetId === this.lastSheetId) {
+      this.lastSheetId--
+    }
+
+    return true
   }
 
   /**
@@ -285,6 +312,11 @@ export class SheetMapping {
       } else {
         this.mappingFromCanonicalNameToId.delete(sheetWithConflictingName.canonicalName)
         this.allSheets.delete(sheetWithConflictingName.id)
+
+        if (sheetWithConflictingName.id === this.lastSheetId) {
+          this.lastSheetId--
+        }
+
         mergedWithPlaceholderSheet = sheetWithConflictingName.id
       }
     }
@@ -293,7 +325,7 @@ export class SheetMapping {
     this.mappingFromCanonicalNameToId.delete(currentCanonicalName)
 
     sheet.displayName = newDisplayName
-    this.storeSheetInMappings(sheet)
+    this._storeSheetInMappings(sheet)
     return { previousDisplayName: currentDisplayName, mergedWithPlaceholderSheet }
   }
 
@@ -304,7 +336,7 @@ export class SheetMapping {
    *
    * @internal
    */
-  private storeSheetInMappings(sheet: Sheet): void {
+  private _storeSheetInMappings(sheet: Sheet): void {
     this.allSheets.set(sheet.id, sheet)
     this.mappingFromCanonicalNameToId.set(sheet.canonicalName, sheet.id)
   }

test/unit/cruds/adding-sheet.spec.ts

@@ -326,7 +326,7 @@ describe('recalculates formulas after adding new sheet (issue #1116)', () => {
       const engine = HyperFormula.buildFromSheets({
         'FirstSheet': [
           ['=MEDIAN(A2)', '=MEDIAN(B2)', '=MEDIAN(C2)', '=MEDIAN(D2)'],
-          [`='NewSheet'!A1:A1`, `='NewSheet'!A1:B2`, `='NewSheet'!A1:A3`, `='NewSheet'!A1:A4`],
+          ['=\'NewSheet\'!A1:A1', '=\'NewSheet\'!A1:B2', '=\'NewSheet\'!A1:A3', '=\'NewSheet\'!A1:A4'],
         ],
       }, {useArrayArithmetic: false})
       const sheet1Id = engine.getSheetId('FirstSheet')!
@@ -349,7 +349,7 @@ describe('recalculates formulas after adding new sheet (issue #1116)', () => {
       const engine = HyperFormula.buildFromSheets({
         'FirstSheet': [
           ['=SUM(A2)', '=SUM(B2)', '=SUM(C2)', '=SUM(D2)'],
-          [`='NewSheet'!A1:A1`, `='NewSheet'!A1:B2`, `='NewSheet'!A1:A3`, `='NewSheet'!A1:A4`],
+          ['=\'NewSheet\'!A1:A1', '=\'NewSheet\'!A1:B2', '=\'NewSheet\'!A1:A3', '=\'NewSheet\'!A1:A4'],
         ],
       }, {useArrayArithmetic: false})
       const sheet1Id = engine.getSheetId('FirstSheet')!
@@ -370,7 +370,7 @@ describe('recalculates formulas after adding new sheet (issue #1116)', () => {
 
     it('function calling a named expression', () => {
       const engine = HyperFormula.buildFromSheets({
-        'FirstSheet': [[`='NewSheet'!A1:A4`]],
+        'FirstSheet': [['=\'NewSheet\'!A1:A4']],
       }, {useArrayArithmetic: false}, [
         { name: 'ExprA', expression: '=MEDIAN(NewSheet!$A$1:$A$1)' },
         { name: 'ExprB', expression: '=MEDIAN(NewSheet!$A$1:$A$2)' },

test/unit/cruds/removing-sheet.spec.ts

@@ -10,22 +10,20 @@ import {
   detailedError,
   detailedErrorWithOrigin,
   expectArrayWithSameContent,
-  expectEngineToBeTheSameAs,
-  expectReferenceToHaveRefError,
   extractReference,
 } from '../testUtils'
 
 describe('Removing sheet - checking if its possible', () => {
   it('no if theres no such sheet', () => {
     const engine = HyperFormula.buildFromArray([[]])
 
-    expect(engine.isItPossibleToRemoveSheet(1)).toEqual(false)
+    expect(engine.isItPossibleToRemoveSheet(1)).toBe(false)
   })
 
   it('yes otherwise', () => {
     const engine = HyperFormula.buildFromArray([[]])
 
-    expect(engine.isItPossibleToRemoveSheet(0)).toEqual(true)
+    expect(engine.isItPossibleToRemoveSheet(0)).toBe(true)
   })
 })
 
@@ -116,6 +114,70 @@ describe('remove sheet', () => {
     expect(engine.getCellValue(adr('A2', 1))).toBe(2)
     expect(engine.getCellValue(adr('A3', 1))).toBe(3)
   })
+
+  it('converts sheet to placeholder if other sheet depends on it', () => {
+    const engine = HyperFormula.buildFromSheets({
+      Sheet1: [[42]],
+      Sheet2: [['=Sheet1!A1']],
+    })
+
+    const sheet1Id = engine.getSheetId('Sheet1')!
+
+    engine.removeSheet(sheet1Id)
+
+    expect(engine.sheetMapping.hasSheetWithId(sheet1Id, { includePlaceholders: false })).toBe(false)
+    expect(engine.sheetMapping.hasSheetWithId(sheet1Id, { includePlaceholders: true })).toBe(true)
+  })
+
+  it('removes sheet completely if nothing depends on it', () => {
+    const engine = HyperFormula.buildFromSheets({
+      Sheet1: [[42]],
+      Sheet2: [[100]],
+    })
+
+    const sheet1Id = engine.getSheetId('Sheet1')!
+
+    engine.removeSheet(sheet1Id)
+
+    expect(engine.sheetMapping.hasSheetWithId(sheet1Id, { includePlaceholders: false })).toBe(false)
+    expect(engine.sheetMapping.hasSheetWithId(sheet1Id, { includePlaceholders: true })).toBe(false)
+  })
+
+  it('removes the placeholder sheet if nothing depends on it any longer', () => {
+    const engine = HyperFormula.buildFromSheets({
+      Sheet1: [[42]],
+      Sheet2: [['=Sheet1!A1']],
+    })
+
+    const sheet1Id = engine.getSheetId('Sheet1')!
+    const sheet2Id = engine.getSheetId('Sheet2')!
+
+    engine.removeSheet(sheet1Id)
+
+    expect(engine.sheetMapping.hasSheetWithId(sheet1Id, { includePlaceholders: false })).toBe(false)
+    expect(engine.sheetMapping.hasSheetWithId(sheet1Id, { includePlaceholders: true })).toBe(true)
+
+    engine.setCellContents(adr('A1', sheet2Id), 100)
+
+    expect(engine.sheetMapping.hasSheetWithId(sheet1Id, { includePlaceholders: false })).toBe(false)
+    expect(engine.sheetMapping.hasSheetWithId(sheet1Id, { includePlaceholders: true })).toBe(false)
+  })
+
+  it('decreases lastSheetId if removed sheet was the last one', () => {
+    const engine = HyperFormula.buildFromSheets({
+      Sheet1: [[1]],
+      Sheet2: [[2]],
+    })
+
+    const sheet2Id = engine.getSheetId('Sheet2')!
+
+    engine.removeSheet(sheet2Id)
+
+    engine.addSheet('Sheet3')
+    const sheet3Id = engine.getSheetId('Sheet3')!
+
+    expect(sheet3Id).toBe(sheet2Id) // new sheet reuses the ID
+  })
 })
 
 describe('remove sheet - adjust edges', () => {
@@ -149,6 +211,7 @@ describe('remove sheet - adjust edges', () => {
 
     const a1From0 = engine.addressMapping.getCell(adr('A1'))
     const a1From1 = engine.addressMapping.getCell(adr('A1', 1))
+
     expect(engine.graph.existsEdge(a1From1!, a1From0!)).toBe(true)
 
     engine.removeSheet(1)
@@ -221,7 +284,7 @@ describe('remove sheet - adjust formula dependencies', () => {
 
 })
 
-describe('remove sheet - issue #1116', () => {
+describe('removeSheet() recalculates formulas (issue #1116)', () => {
   it('returns REF error after removing sheet referenced without quotes', () => {
     const engine = HyperFormula.buildEmpty()
     const table1Name = 'table1'
@@ -727,6 +790,7 @@ describe('remove sheet - adjust matrix mapping', () => {
         ['=TRANSPOSE(A1:B1)'],
       ],
     })
+
     expect(engine.arrayMapping.getArray(AbsoluteCellRange.spanFrom(adr('A2'), 1, 2))).toBeInstanceOf(ArrayFormulaVertex)
 
     engine.removeSheet(0)
@@ -746,7 +810,7 @@ describe('remove sheet - adjust column index', () => {
 
     engine.removeSheet(0)
 
-    expect(removeSheetSpy).toHaveBeenCalled()
+    expect(removeSheetSpy).toHaveBeenCalledWith(0)
     expectArrayWithSameContent([], index.getValueIndex(0, 0, 1).index)
   })
 })