refactor(express-engine, hapi-engine): use CommonEngine (#1246)

vikerman · web-flow · commit e8896528bfd9 · 2019-09-18T23:11:25.000-07:00
* refactor(express-engine, hapi-engine): use CommonEngine

* fix: avoid memory leak through opts.providers

Don't mutate `options` being passed into the render call(especially concatting to the providers array).

Mark `options` as `Readonly` so that it is not accidentally mutated in the future.
diff --git a/modules/aspnetcore-engine/src/main.ts b/modules/aspnetcore-engine/src/main.ts
@@ -91,8 +91,8 @@ function _getUniversalData(doc: Document): UniversalData {
   };
 }
 
-export function ngAspnetCoreEngine(options: IEngineOptions): Promise<IEngineRenderResult> {
-
+export function ngAspnetCoreEngine(options: Readonly<IEngineOptions>)
+    : Promise<IEngineRenderResult> {
   if (!options.appSelector) {
     const selector = `" appSelector: '<${appSelector}></${appSelector}>' "`;
     throw new Error(`appSelector is required! Pass in ${selector},
@@ -119,9 +119,9 @@ export function ngAspnetCoreEngine(options: IEngineOptions): Promise<IEngineRend
         throw new Error('You must pass in a NgModule or NgModuleFactory to be bootstrapped');
       }
 
-      options.providers = options.providers || [];
+      let extraProviders = options.providers || [];
 
-      const extraProviders = options.providers.concat(getReqResProviders(options.request.origin,
+      extraProviders = extraProviders.concat(getReqResProviders(options.request.origin,
         options.request.data.request));
 
       getFactory(moduleOrFactory, compiler)
diff --git a/modules/common/engine/src/engine.ts b/modules/common/engine/src/engine.ts
@@ -31,7 +31,7 @@ export class CommonEngine {
   private factoryCacheMap = new Map<Type<{}>, NgModuleFactory<{}>>();
   private templateCache: {[key: string]: string} = {};
 
-  constructor(private moduleOrFactory: Type<{}> | NgModuleFactory<{}>,
+  constructor(private moduleOrFactory?: Type<{}> | NgModuleFactory<{}>,
               private providers: StaticProvider[] = []) {}
 
   /**
@@ -53,14 +53,14 @@ export class CommonEngine {
       }
     ];
 
-    const factory = await this.getFactory();
+    const moduleOrFactory = this.moduleOrFactory || opts.bootstrap;
+    const factory = await this.getFactory(moduleOrFactory);
     return renderModuleFactory(factory, {extraProviders});
   }
 
   /** Return the factory for a given engine instance */
-  getFactory(): Promise<NgModuleFactory<{}>> {
+  getFactory(moduleOrFactory: Type<{}> | NgModuleFactory<{}>): Promise<NgModuleFactory<{}>> {
     // If module has been compiled AoT
-    const moduleOrFactory = this.moduleOrFactory;
     if (moduleOrFactory instanceof NgModuleFactory) {
       return Promise.resolve(moduleOrFactory);
     } else {
diff --git a/modules/express-engine/BUILD.bazel b/modules/express-engine/BUILD.bazel
@@ -10,6 +10,7 @@ ng_module(
     ]),
     module_name = "@nguniversal/express-engine",
     deps = [
+        "//modules/common/engine",
         "//modules/express-engine/tokens",
         "@npm//@angular/compiler",
         "@npm//@angular/core",
diff --git a/modules/express-engine/src/main.ts b/modules/express-engine/src/main.ts
@@ -8,15 +8,8 @@
 import * as fs from 'fs';
 import { Request, Response } from 'express';
 
-import { NgModuleFactory, Type, CompilerFactory, Compiler, StaticProvider } from '@angular/core';
-import { ResourceLoader } from '@angular/compiler';
-import {
-  INITIAL_CONFIG,
-  renderModuleFactory,
-  platformDynamicServer
-} from '@angular/platform-server';
-
-import { FileLoader } from './file-loader';
+import { NgModuleFactory, Type, StaticProvider } from '@angular/core';
+import { ɵCommonEngine as CommonEngine } from '@nguniversal/common/engine';
 import { REQUEST, RESPONSE } from '@nguniversal/express-engine/tokens';
 
 /**
@@ -42,101 +35,39 @@ export interface RenderOptions extends NgSetupOptions {
  */
 const templateCache: { [key: string]: string } = {};
 
-/**
- * Map of Module Factories
- */
-const factoryCacheMap = new Map<Type<{}>, NgModuleFactory<{}>>();
-
 /**
  * This is an express engine for handling Angular Applications
  */
-export function ngExpressEngine(setupOptions: NgSetupOptions) {
-
-  const compilerFactory: CompilerFactory = platformDynamicServer().injector.get(CompilerFactory);
-  const compiler: Compiler = compilerFactory.createCompiler([
-    {
-      providers: [
-        { provide: ResourceLoader, useClass: FileLoader, deps: [] }
-      ]
-    }
-  ]);
+export function ngExpressEngine(setupOptions: Readonly<NgSetupOptions>) {
+  const engine = new CommonEngine(setupOptions.bootstrap, setupOptions.providers);
 
   return function (filePath: string,
-                   options: RenderOptions,
+                   options: Readonly<RenderOptions>,
                    callback: (err?: Error | null, html?: string) => void) {
-
-    options.providers = options.providers || [];
-
     try {
-      const moduleOrFactory = options.bootstrap || setupOptions.bootstrap;
-
-      if (!moduleOrFactory) {
+      if (!setupOptions.bootstrap && !options.bootstrap) {
         throw new Error('You must pass in a NgModule or NgModuleFactory to be bootstrapped');
       }
 
-      setupOptions.providers = setupOptions.providers || [];
-
       const req = options.req;
-      const extraProviders = setupOptions.providers.concat(
-        options.providers,
-        getReqResProviders(options.req, options.res),
-        [
-          {
-            provide: INITIAL_CONFIG,
-            useValue: {
-              document: options.document || getDocument(filePath),
-              url: options.url || `${req.protocol}://${(req.get('host') || '')}${req.originalUrl}`
-            }
-          }
-        ]);
 
-      getFactory(moduleOrFactory, compiler)
-        .then(factory => {
-          return renderModuleFactory(factory, {
-            extraProviders
-          });
-        })
-        .then((html: string) => {
-          callback(null, html);
-        }, (err) => {
-          callback(err);
-        });
-    } catch (err) {
-      callback(err);
-    }
-  };
-}
+      const renderOptions: RenderOptions = Object.assign({}, options);
 
-/**
- * Get a factory from a bootstrapped module/ module factory
- */
-function getFactory(
-  moduleOrFactory: Type<{}> | NgModuleFactory<{}>, compiler: Compiler
-): Promise<NgModuleFactory<{}>> {
-  return new Promise<NgModuleFactory<{}>>((resolve, reject) => {
-    // If module has been compiled AoT
-    if (moduleOrFactory instanceof NgModuleFactory) {
-      resolve(moduleOrFactory);
-      return;
-    } else {
-      let moduleFactory = factoryCacheMap.get(moduleOrFactory);
+      renderOptions.url =
+        options.url || `${req.protocol}://${(req.get('host') || '')}${req.originalUrl}`;
+      renderOptions.document = options.document || getDocument(filePath);
 
-      // If module factory is cached
-      if (moduleFactory) {
-        resolve(moduleFactory);
-        return;
-      }
+      renderOptions.providers = options.providers || [];
+      renderOptions.providers =
+        renderOptions.providers.concat(getReqResProviders(options.req, options.res));
 
-      // Compile the module and cache it
-      compiler.compileModuleAsync(moduleOrFactory)
-        .then((factory) => {
-          factoryCacheMap.set(moduleOrFactory, factory);
-          resolve(factory);
-        }, (err => {
-          reject(err);
-        }));
+      engine.render(renderOptions)
+        .then(html => callback(null, html))
+        .catch(callback);
+    } catch (err) {
+      callback(err);
     }
-  });
+  };
 }
 
 /**
diff --git a/modules/hapi-engine/BUILD.bazel b/modules/hapi-engine/BUILD.bazel
@@ -10,6 +10,7 @@ ng_module(
     ]),
     module_name = "@nguniversal/hapi-engine",
     deps = [
+        "//modules/common/engine",
         "//modules/hapi-engine/tokens",
         "@npm//@angular/platform-server",
         "@npm//@types/hapi",
diff --git a/modules/hapi-engine/src/main.ts b/modules/hapi-engine/src/main.ts
@@ -8,15 +8,8 @@
 import * as fs from 'fs';
 import { Request, ResponseToolkit } from 'hapi';
 
-import { NgModuleFactory, Type, CompilerFactory, Compiler, StaticProvider } from '@angular/core';
-import { ResourceLoader } from '@angular/compiler';
-import {
-  INITIAL_CONFIG,
-  renderModuleFactory,
-  platformDynamicServer
-} from '@angular/platform-server';
-
-import { FileLoader } from './file-loader';
+import { NgModuleFactory, Type, StaticProvider } from '@angular/core';
+import { ɵCommonEngine as CommonEngine } from '@nguniversal/common/engine';
 import { REQUEST, RESPONSE } from '@nguniversal/hapi-engine/tokens';
 
 /**
@@ -43,34 +36,23 @@ export interface RenderOptions extends NgSetupOptions {
 const templateCache: { [key: string]: string } = {};
 
 /**
- * Map of Module Factories
+ * The CommonEngine with module to facory map in case of JIT.
  */
-const factoryCacheMap = new Map<Type<{}>, NgModuleFactory<{}>>();
+const commonEngine = new CommonEngine(undefined);
 
 /**
  * This is an express engine for handling Angular Applications
  */
-export function ngHapiEngine(options: RenderOptions) {
-
+export function ngHapiEngine(options: Readonly<RenderOptions>) {
   const req = options.req;
-  const compilerFactory: CompilerFactory = platformDynamicServer().injector.get(CompilerFactory);
-  const compiler: Compiler = compilerFactory.createCompiler([
-    {
-      providers: [
-        { provide: ResourceLoader, useClass: FileLoader, deps: [] }
-      ]
-    }
-  ]);
-
   if (req.raw.req.url === undefined) {
     return Promise.reject(new Error('url is undefined'));
   }
 
   const protocol = req.server.info.protocol;
   const filePath = <string> req.raw.req.url;
-  const url = `${protocol}://${req.info.host}${req.path}`;
 
-  options.providers = options.providers || [];
+  const renderOptions: RenderOptions = Object.assign({}, options);
 
   return new Promise((resolve, reject) => {
     const moduleOrFactory = options.bootstrap;
@@ -79,52 +61,13 @@ export function ngHapiEngine(options: RenderOptions) {
       return reject(new Error('You must pass in a NgModule or NgModuleFactory to be bootstrapped'));
     }
 
-    const extraProviders = options.providers!.concat(
-      options.providers!,
-      getReqProviders(options.req),
-      [
-        {
-          provide: INITIAL_CONFIG,
-          useValue: {
-            document: options.document || getDocument(filePath),
-            url: options.url || url
-          }
-        }
-      ]);
-
-    getFactory(moduleOrFactory, compiler)
-      .then(factory => renderModuleFactory(factory, {extraProviders}))
-      .then(resolve, reject);
-  });
-}
-
-/**
- * Get a factory from a bootstrapped module/ module factory
- */
-function getFactory(
-  moduleOrFactory: Type<{}> | NgModuleFactory<{}>, compiler: Compiler
-): Promise<NgModuleFactory<{}>> {
-  return new Promise<NgModuleFactory<{}>>((resolve, reject) => {
-    // If module has been compiled AoT
-    if (moduleOrFactory instanceof NgModuleFactory) {
-      resolve(moduleOrFactory);
-      return;
-    } else {
-      let moduleFactory = factoryCacheMap.get(moduleOrFactory);
+    renderOptions.url = options.url || `${protocol}://${req.info.host}${req.path}`;
+    renderOptions.document = options.document || getDocument(filePath);
 
-      // If module factory is cached
-      if (moduleFactory) {
-        resolve(moduleFactory);
-        return;
-      }
+    renderOptions.providers = options.providers || [];
+    renderOptions.providers = renderOptions.providers.concat(getReqProviders(options.req));
 
-      // Compile the module and cache it
-      compiler.compileModuleAsync(moduleOrFactory)
-        .then((factory) => {
-          factoryCacheMap.set(moduleOrFactory, factory);
-          resolve(factory);
-        }, reject);
-    }
+    commonEngine.render(renderOptions).then(resolve, reject);
   });
 }
 
diff --git a/package.json b/package.json
@@ -69,9 +69,9 @@
     "bazel:lint": "yarn bazel:format --lint=warn",
     "bazel:lint-fix": "yarn bazel:format --lint=fix",
     "bazel:build": "bazel build //...",
-    "test": "bazel test //...",
+    "test": "bazel test //modules/...",
     "build:watch": "ibazel build //...",
-    "test:watch": "ibazel test //...",
+    "test:watch": "ibazel test //modules/...",
     "preinstall": "node ./tools/npm/check-npm.js",
     "bazel": "bazel"
   },
