refactor: change how the chart wrapper is created

libs/angular-google-charts/src/lib/components/chart-base/chart-base.component.ts

@@ -44,7 +44,7 @@ export class ChartBase {
   public chartWrapper: google.visualization.ChartWrapper | null;
 
   /**
-   * Emits every time the `ChartWrapper` is recreated.
+   * Emits after the `ChartWrapper` is created, but before the chart is drawn for the first time.
    */
   public wrapperReady$: Observable<google.visualization.ChartWrapper>;
 }

libs/angular-google-charts/src/lib/components/chart-wrapper/chart-wrapper.component.spec.ts

@@ -1,6 +1,6 @@
 import { SimpleChange } from '@angular/core';
 import { async, ComponentFixture, TestBed } from '@angular/core/testing';
-import { of } from 'rxjs';
+import { EMPTY, of } from 'rxjs';
 
 import { ChartType } from '../../models/chart-type.model';
 import { ScriptLoaderService } from '../../script-loader/script-loader.service';
@@ -47,11 +47,6 @@ describe('ChartWrapperComponent', () => {
     }).compileComponents();
   }));
 
-  beforeEach(() => {
-    const scriptLoaderService = TestBed.inject(ScriptLoaderService) as jest.Mocked<ScriptLoaderService>;
-    scriptLoaderService.loadChartPackages.mockReturnValue(of(null));
-  });
-
   beforeEach(() => {
     fixture = TestBed.createComponent(ChartWrapperComponent);
     component = fixture.componentInstance;
@@ -63,51 +58,157 @@ describe('ChartWrapperComponent', () => {
     expect(component).toBeTruthy();
   });
 
-  it('should load google charts', () => {
-    component.ngOnInit();
+  describe('ngOnInit', () => {
+    it('should load the google charts package', () => {
+      const scriptLoaderService = TestBed.inject(ScriptLoaderService) as jest.Mocked<ScriptLoaderService>;
+      scriptLoaderService.loadChartPackages.mockReturnValue(EMPTY);
 
-    const scriptLoaderService = TestBed.inject(ScriptLoaderService) as jest.Mocked<ScriptLoaderService>;
-    expect(scriptLoaderService.loadChartPackages).toHaveBeenCalled();
-  });
+      component.ngOnInit();
 
-  it('should draw a chart using the provided specs', () => {
-    const specs = { chartType: ChartType.AreaChart, dataTable: [] };
-    component.specs = specs;
-    component.ngOnInit();
+      expect(scriptLoaderService.loadChartPackages).toHaveBeenCalled();
+    });
 
-    expect(visualizationMock.ChartWrapper).toHaveBeenCalled();
-    expect(chartWrapperMock.setChartType).toBeCalledWith(specs.chartType);
-    expect(chartWrapperMock.setDataTable).toHaveBeenCalledWith(specs.dataTable);
-    expect(chartWrapperMock.draw).toHaveBeenCalled();
-  });
+    it('should create the chart wrapper using the provided specs', () => {
+      const scriptLoaderService = TestBed.inject(ScriptLoaderService) as jest.Mocked<ScriptLoaderService>;
+      scriptLoaderService.loadChartPackages.mockReturnValue(of(null));
 
-  it('should redraw the chart if the specs change', () => {
-    const specs = { chartType: ChartType.AreaChart, dataTable: [] } as google.visualization.ChartSpecs;
-    component.specs = specs;
-    component.ngOnInit();
+      const specs = { chartType: ChartType.AreaChart, dataTable: [] };
+      component.specs = specs;
+      component.ngOnInit();
 
-    const newSpecs = { ...specs, chartType: ChartType.GeoChart };
-    changeSpecs(newSpecs);
+      expect(visualizationMock.ChartWrapper).toHaveBeenCalledWith(expect.objectContaining(specs));
+    });
 
-    expect(chartWrapperMock.draw).toHaveBeenCalledTimes(2);
-  });
+    it('should not throw if the specs are `null`', async(() => {
+      const scriptLoaderService = TestBed.inject(ScriptLoaderService) as jest.Mocked<ScriptLoaderService>;
+      scriptLoaderService.loadChartPackages.mockReturnValue(of(null));
+      component.specs = null;
 
-  it("should not redraw the chart if the specs didn't change", () => {
-    const specs = { chartType: ChartType.AreaChart, dataTable: [] } as google.visualization.ChartSpecs;
-    component.specs = specs;
-    component.ngOnInit();
+      expect(() => component.ngOnInit()).not.toThrow();
+    }));
 
-    component.ngOnChanges({});
+    it('should not use container or containerId if present in the chart specs', () => {
+      const scriptLoaderService = TestBed.inject(ScriptLoaderService) as jest.Mocked<ScriptLoaderService>;
+      scriptLoaderService.loadChartPackages.mockReturnValue(of(null));
+
+      const specs = { chartType: ChartType.AreaChart, container: { innerHTML: '' } as HTMLElement, containerId: 'test' };
+      component.specs = specs;
+      component.ngOnInit();
+
+      expect(visualizationMock.ChartWrapper).not.toHaveBeenCalledWith(expect.objectContaining({ container: specs.container }));
+      expect(visualizationMock.ChartWrapper).not.toHaveBeenCalledWith(expect.objectContaining({ containerId: specs.containerId }));
+    });
+
+    it('should register chart wrapper event handlers', () => {
+      const scriptLoaderService = TestBed.inject(ScriptLoaderService) as jest.Mocked<ScriptLoaderService>;
+      scriptLoaderService.loadChartPackages.mockReturnValue(of(null));
+
+      component.specs = { chartType: ChartType.AreaChart };
+
+      component.ngOnInit();
 
-    expect(chartWrapperMock.draw).toHaveBeenCalledTimes(1);
+      expect(visualizationMock.events.removeAllListeners).toHaveBeenCalled();
+      expect(visualizationMock.events.addListener).toHaveBeenCalledWith(chartWrapperMock, 'ready', expect.any(Function));
+      expect(visualizationMock.events.addListener).toHaveBeenCalledWith(chartWrapperMock, 'error', expect.any(Function));
+      expect(visualizationMock.events.addListener).toHaveBeenCalledWith(chartWrapperMock, 'select', expect.any(Function));
+    });
+
+    it('should emit ready event', () => {
+      const scriptLoaderService = TestBed.inject(ScriptLoaderService) as jest.Mocked<ScriptLoaderService>;
+      scriptLoaderService.loadChartPackages.mockReturnValue(of(null));
+
+      component.specs = { chartType: ChartType.AreaChart };
+
+      const readySpy = jest.fn();
+      component.wrapperReady$.subscribe(event => readySpy(event));
+
+      component.ngOnInit();
+
+      expect(readySpy).toHaveBeenCalledWith(chartWrapperMock);
+    });
+
+    it('should draw the chart', () => {
+      const scriptLoaderService = TestBed.inject(ScriptLoaderService) as jest.Mocked<ScriptLoaderService>;
+      scriptLoaderService.loadChartPackages.mockReturnValue(of(null));
+
+      component.specs = { chartType: ChartType.AreaChart };
+
+      component.ngOnInit();
+
+      expect(chartWrapperMock.draw).toHaveBeenCalled();
+    });
   });
 
-  it('should ignore `container` and `containerId` if given', () => {
-    const specs = { chartType: ChartType.AreaChart, containerId: 'test', container: {} } as google.visualization.ChartSpecs;
-    component.specs = specs;
-    component.ngOnInit();
+  describe('ngOnChanges', () => {
+    beforeEach(() => {
+      component['wrapper'] = chartWrapperMock as any;
+      component['initialized'] = true;
+    });
+
+    it('should not throw if the wrapper is not yet initialized', () => {
+      component['wrapper'] = null;
+      component['initialized'] = false;
+
+      const specs = {
+        chartType: ChartType.AreaChart
+      };
+
+      expect(() => changeSpecs(specs)).not.toThrow();
+    });
+
+    it('should update the chart wrapper with the provided specs', () => {
+      const specs = {
+        chartType: ChartType.AreaChart,
+        dataSourceUrl: 'www.data.de',
+        dataTable: [],
+        options: { test: 'any' },
+        query: 'query',
+        refreshInterval: 100,
+        view: 'testview'
+      } as google.visualization.ChartSpecs;
+
+      changeSpecs(specs);
+
+      expect(chartWrapperMock.setChartType).toHaveBeenCalledWith(specs.chartType);
+      expect(chartWrapperMock.setDataSourceUrl).toHaveBeenCalledWith(specs.dataSourceUrl);
+      expect(chartWrapperMock.setDataTable).toHaveBeenCalledWith(specs.dataTable);
+      expect(chartWrapperMock.setOptions).toHaveBeenCalledWith(specs.options);
+      expect(chartWrapperMock.setQuery).toHaveBeenCalledWith(specs.query);
+      expect(chartWrapperMock.setRefreshInterval).toHaveBeenCalledWith(specs.refreshInterval);
+      expect(chartWrapperMock.setView).toHaveBeenCalledWith(specs.view);
+    });
+
+    it('should ignore `container` and `containerId` if given', () => {
+      const specs = { containerId: 'test', container: {} } as google.visualization.ChartSpecs;
+      component.specs = specs;
+
+      expect(chartWrapperMock.setContainerId).not.toHaveBeenCalled();
+    });
+
+    it('should not throw if the specs are `null`', () => {
+      expect(() => changeSpecs(null)).not.toThrow();
+
+      expect(chartWrapperMock.draw).toHaveBeenCalled();
+    });
+
+    it('should redraw the chart if the specs change', () => {
+      const specs = { chartType: ChartType.AreaChart } as google.visualization.ChartSpecs;
+      component.specs = specs;
+
+      const newSpecs = { ...specs, chartType: ChartType.GeoChart };
+      changeSpecs(newSpecs);
+
+      expect(chartWrapperMock.draw).toHaveBeenCalled();
+    });
+
+    it("should not redraw the chart if the specs didn't change", () => {
+      const specs = { chartType: ChartType.AreaChart } as google.visualization.ChartSpecs;
+      component.specs = specs;
+
+      component.ngOnChanges({});
 
-    expect(chartWrapperMock.setContainerId).not.toBeCalled();
+      expect(chartWrapperMock.draw).not.toHaveBeenCalled();
+    });
   });
 
   describe('chart', () => {
@@ -122,34 +223,17 @@ describe('ChartWrapperComponent', () => {
     });
 
     it('should return the chart wrapper', () => {
-      const specs = { chartType: ChartType.AreaChart, dataTable: [] } as google.visualization.ChartSpecs;
-      component.specs = specs;
-      component.ngOnInit();
+      component['wrapper'] = chartWrapperMock as any;
 
       const wrapper = component.chartWrapper;
       expect(wrapper).toBe(chartWrapperMock);
     });
   });
 
   describe('events', () => {
-    const specs = { chartType: ChartType.AreaChart } as google.visualization.ChartSpecs;
-
     beforeEach(() => {
-      component.specs = specs;
-    });
-
-    it('should remove all event handlers before redrawing the chart', () => {
-      component.ngOnInit();
-
-      expect(visualizationMock.events.removeAllListeners).toHaveBeenCalled();
-    });
-
-    it('should register chart wrapper event handlers', () => {
-      component.ngOnInit();
-
-      expect(visualizationMock.events.addListener).toHaveBeenCalledWith(chartWrapperMock, 'ready', expect.any(Function));
-      expect(visualizationMock.events.addListener).toHaveBeenCalledWith(chartWrapperMock, 'error', expect.any(Function));
-      expect(visualizationMock.events.addListener).toHaveBeenCalledWith(chartWrapperMock, 'select', expect.any(Function));
+      const service = TestBed.inject(ScriptLoaderService) as jest.Mocked<ScriptLoaderService>;
+      service.loadChartPackages.mockReturnValueOnce(of(null));
     });
 
     it('should emit ready event after the chart is ready', () => {

libs/angular-google-charts/src/lib/components/chart-wrapper/chart-wrapper.component.ts

@@ -69,10 +69,22 @@ export class ChartWrapperComponent implements ChartBase, OnChanges, OnInit {
   public ngOnInit() {
     // We don't need to load any chart packages, the chart wrapper will handle this else for us
     this.scriptLoaderService.loadChartPackages().subscribe(() => {
+      if (!this.specs) {
+        this.specs = {} as google.visualization.ChartSpecs;
+      }
+
+      const { containerId, container, ...specs } = this.specs;
+
       // Only ever create the wrapper once to allow animations to happen if something changes.
-      this.wrapper = new google.visualization.ChartWrapper();
-      this.createChart();
+      this.wrapper = new google.visualization.ChartWrapper({
+        ...specs,
+        container: this.element.nativeElement
+      });
+      this.registerChartEvents();
+
+      this.wrapperReadySubject.next(this.wrapper);
 
+      this.drawChart();
       this.initialized = true;
     });
   }
@@ -83,11 +95,12 @@ export class ChartWrapperComponent implements ChartBase, OnChanges, OnInit {
     }
 
     if (changes.specs) {
-      this.createChart();
+      this.updateChart();
+      this.drawChart();
     }
   }
 
-  private createChart() {
+  private updateChart() {
     if (!this.specs) {
       // When creating the wrapper with empty specs, the google charts library will show an error
       // If we don't do this, a javascript error will be thrown, which is not as visible to the user
@@ -102,11 +115,10 @@ export class ChartWrapperComponent implements ChartBase, OnChanges, OnInit {
     this.wrapper.setOptions(this.specs.options);
     this.wrapper.setRefreshInterval(this.specs.refreshInterval);
     this.wrapper.setView(this.specs.view);
+  }
 
-    this.registerChartEvents();
-
-    this.wrapperReadySubject.next(this.wrapper);
-    this.wrapper.draw(this.element.nativeElement);
+  private drawChart() {
+    this.wrapper.draw();
   }
 
   private registerChartEvents() {