Merge pull request #639 from Financial-Times/publicpath

Change behaviour of AssetLoader's `publicPath` property
Financial-Times · Oct 29, 2019 · 302b027 · 302b027
2 parents b1429e3 + 2d868cd
commit 302b027
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 35 deletions.
diff --git a/examples/ssr-with-hydration/app/server.js b/examples/ssr-with-hydration/app/server.js
@@ -10,7 +10,7 @@ import { getInitialProps, getDependencies } from '../libs/ssr/server'
 const app = express()
 const port = 3000
 const assets = new AssetLoader({
-  publicPath: '/assets',
+  publicPath: '/assets/',
   fileSystemPath: path.resolve('./dist'),
   manifestFileName: 'manifest.json'
 })

diff --git a/packages/dotcom-server-asset-loader/src/AssetLoader.ts b/packages/dotcom-server-asset-loader/src/AssetLoader.ts
@@ -33,7 +33,7 @@ export interface AssetLoaderOptions {
 }
 
 const defaultOptions: AssetLoaderOptions = {
-  publicPath: '',
+  publicPath: '/',
   manifestFileName: 'manifest.json',
   fileSystemPath: path.resolve('./public'),
   cacheFileContents: false
@@ -59,10 +59,10 @@ export class AssetLoader {
   public options: AssetLoaderOptions
   public manifest: TManifest
 
-  constructor(userOptions: AssetLoaderOptions) {
+  constructor(userOptions?: AssetLoaderOptions) {
     this.options = { ...defaultOptions, ...userOptions }
     this.manifest =
-      userOptions.manifest ||
+      this.options.manifest ||
       loadManifest(path.resolve(this.options.fileSystemPath, this.options.manifestFileName))
   }
 
@@ -117,8 +117,7 @@ export class AssetLoader {
   //
 
   formatPublicURL(hashedAsset: string): string {
-    // Do not use path.join() as separator is platform specific, e.g. "\" on Windows
-    return this.options.publicPath ? `${this.options.publicPath}/${hashedAsset}` : hashedAsset
+    return path.posix.join(this.options.publicPath, hashedAsset)
   }
 
   formatFileSystemPath(hashedAsset: string): string {

diff --git a/packages/dotcom-server-asset-loader/src/__test__/AssetLoader.spec.ts b/packages/dotcom-server-asset-loader/src/__test__/AssetLoader.spec.ts
@@ -15,19 +15,18 @@ jest.mock('../helpers/loadFile', () => {
   }
 })
 
-function createAssetLoader({
-  publicPath = 'public/assets',
-  fileSystemPath = '/internal/path/to/assets',
-  ...otherOptions
-}: AssetLoaderOptions = {}) {
-  return new AssetLoader({ publicPath, fileSystemPath, ...otherOptions })
+function createAssetLoader(options?: AssetLoaderOptions) {
+  return new AssetLoader(options)
 }
 
 describe('dotcom-server-asset-loader/src/AssetLoader', () => {
   let loader
 
   beforeEach(() => {
-    loader = createAssetLoader()
+    loader = createAssetLoader({
+      publicPath: '/public/assets/',
+      fileSystemPath: '/internal/path/to/assets'
+    })
   })
 
   afterEach(() => {
@@ -56,6 +55,8 @@ describe('dotcom-server-asset-loader/src/AssetLoader', () => {
     })
   })
 
+
+
   describe('.matchAssets()', () => {
     it('returns an array of matching file names from the manifest', () => {
       const a = loader.matchAssets(/main/)
@@ -85,13 +86,13 @@ describe('dotcom-server-asset-loader/src/AssetLoader', () => {
   describe('getPublicURLOfHashedAssetsMatching(pattern', () => {
     it('returns the public urls of hashed assets whose entry file name matches the supplied pattern', () => {
       const a = loader.getPublicURLOfHashedAssetsMatching(/main/)
-      expect(a).toEqual(['public/assets/main.12345.bundle.js'])
+      expect(a).toEqual(['/public/assets/main.12345.bundle.js'])
 
       const b = loader.getPublicURLOfHashedAssetsMatching('main')
-      expect(b).toEqual(['public/assets/main.12345.bundle.js'])
+      expect(b).toEqual(['/public/assets/main.12345.bundle.js'])
 
       const c = loader.getPublicURLOfHashedAssetsMatching((filename) => filename === 'main.js')
-      expect(c).toEqual(['public/assets/main.12345.bundle.js'])
+      expect(c).toEqual(['/public/assets/main.12345.bundle.js'])
     })
   })
 
@@ -103,15 +104,21 @@ describe('dotcom-server-asset-loader/src/AssetLoader', () => {
   })
 
   describe('.getPublicURL()', () => {
-    it('returns the public path for the requested file', () => {
-      const result = loader.getPublicURL('styles.css')
-      expect(result).toEqual('public/assets/styles.12345.bundle.css')
-    })
-
-    it('should correctly format the url when the public path has not been set', () => {
-      const loader = createAssetLoader({ publicPath: '' })
-      const result = loader.getPublicURL('styles.css')
-      expect(result).toEqual('styles.12345.bundle.css')
+    const tests = [
+      { expected: '/styles.12345.bundle.css' },
+      { publicPath: '', expected: 'styles.12345.bundle.css' },
+      { publicPath: '/', expected: '/styles.12345.bundle.css' },
+      { publicPath: '/lib', expected: '/lib/styles.12345.bundle.css' },
+      { publicPath: '/lib/', expected: '/lib/styles.12345.bundle.css' },
+      { publicPath: '../', expected: '../styles.12345.bundle.css' }
+    ]
+
+    tests.forEach(({ publicPath, expected }) => {
+      it(`publicPath: ${publicPath}`, () => {
+        const loader = typeof publicPath === 'undefined' ? new AssetLoader() : new AssetLoader({ publicPath })
+        const result = loader.getPublicURL('styles.css')
+        expect(result).toEqual(expected)
+      })
     })
   })
 
@@ -196,7 +203,7 @@ describe('dotcom-server-asset-loader/src/AssetLoader', () => {
       expect(result.length).toBe(3)
 
       result.forEach((item) => {
-        expect(item).toMatch(/^\public\/assets\/.+\.js$/)
+        expect(item).toMatch(/^\/public\/assets\/.+\.js$/)
       })
     })
   })
@@ -208,7 +215,7 @@ describe('dotcom-server-asset-loader/src/AssetLoader', () => {
       expect(result.length).toBe(2)
 
       result.forEach((item) => {
-        expect(item).toMatch(/^\public\/assets\/.+\.css$/)
+        expect(item).toMatch(/^\/public\/assets\/.+\.css$/)
       })
     })
   })

diff --git a/packages/dotcom-server-handlebars/src/__test__/HandlebarsRenderer.spec.ts b/packages/dotcom-server-handlebars/src/__test__/HandlebarsRenderer.spec.ts
@@ -80,17 +80,19 @@ describe('dotcom-server-handlebars/src/PageKitHandlebars', () => {
   })
 
   describe('.renderView()', () => {
-    it('can render a template and fire a callback with the result', (done) => {
-      const templateContext = { title: 'Hello World', aside: 'Lorem ipsum' }
+    it('can render a template and fire a callback with the result', () => {
+      return new Promise((done) => {
+        const templateContext = { title: 'Hello World', aside: 'Lorem ipsum' }
 
-      instance.renderView(view, templateContext, (error, result) => {
-        expect(error).toBeNull()
+        instance.renderView(view, templateContext, (error, result) => {
+          expect(error).toBeNull()
 
-        expect(result).toContain('<h1>Hello World</h1>')
-        expect(result).toContain('<aside>Lorem ipsum</aside>')
-        expect(result).toMatch(/<main>.+<\/main>/s)
+          expect(result).toContain('<h1>Hello World</h1>')
+          expect(result).toContain('<aside>Lorem ipsum</aside>')
+          expect(result).toMatch(/<main>.+<\/main>/s)
 
-        done()
+          done()
+        })
       })
     })
   })