Repository: mapbox/batfish
Pull request #174 (2017-09-13 17:29:07)

Remove imports of batfish-internal/context from public modules
@jfurrow Can you please try this branch and see if it fixes the bug you saw?


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5743669..a7dcb2b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,10 @@
 # Changelog
 
+## Head
+
+- [Fix] Remove imports of `batfish-internal/context` from public modules.
+  These break unit tests, or any other reference to the file outside of Batfish's builds.
+
 ## 0.11.0
 
 - [Add] Add `babelPresetEnvOptions` option.
diff --git a/src/webpack/public/prefix-url.js b/src/webpack/public/prefix-url.js
index 84bd76b..ead0904 100644
--- a/src/webpack/public/prefix-url.js
+++ b/src/webpack/public/prefix-url.js
@@ -1,5 +1,11 @@
 // @flow
-import { batfishContext } from 'batfish-internal/context';
+
+// This weirdness, combined with the _configure function below, exists because
+// we don't want a public module to import 'batfish-internal/context' directly.
+// That will make any files that use it incapable of executing outside of
+// Batfish (e.g. they won't work in unit tests).
+let siteBasePath = '';
+let siteOrigin;
 
 // Crude heuristic but probably ok.
 function isAbsoluteUrl(url: string): boolean {
@@ -10,21 +16,18 @@ function prefixUrl(url: string): string {
   if (isAbsoluteUrl(url)) {
     return url;
   }
-  if (
-    batfishContext.selectedConfig.siteBasePath &&
-    url.indexOf(batfishContext.selectedConfig.siteBasePath) === 0
-  ) {
+  if (siteBasePath && url.indexOf(siteBasePath) === 0) {
     return url;
   }
   if (!/^\//.test(url)) url = '/' + url;
-  return batfishContext.selectedConfig.siteBasePath + url;
+  return siteBasePath + url;
 }
 
 function prefixUrlAbsolute(url: string): string {
   if (isAbsoluteUrl(url)) {
     return url;
   }
-  if (!batfishContext.selectedConfig.siteOrigin) {
+  if (!siteOrigin) {
     if (process.env.NODE_ENV !== 'production') {
       throw new Error(
         'The siteOrigin property is not specified in your Batfish configuration. Unable to prefix with absolute path.'
@@ -33,15 +36,16 @@ function prefixUrlAbsolute(url: string): string {
     return url;
   }
   if (!/^\//.test(url)) url = '/' + url;
-  return (
-    batfishContext.selectedConfig.siteOrigin +
-    batfishContext.selectedConfig.siteBasePath +
-    url
-  );
+  return siteOrigin + siteBasePath + url;
 }
 
 function isUrlPrefixed(url: string): boolean {
-  return url.indexOf(batfishContext.selectedConfig.siteBasePath) === 0;
+  return url.indexOf(siteBasePath) === 0;
 }
 
+prefixUrl._configure = (a?: string, b?: string) => {
+  siteBasePath = a || '';
+  siteOrigin = b;
+};
+
 export { prefixUrl, prefixUrlAbsolute, isUrlPrefixed };
diff --git a/src/webpack/router.js b/src/webpack/router.js
index c03701d..ebc5681 100644
--- a/src/webpack/router.js
+++ b/src/webpack/router.js
@@ -7,12 +7,21 @@ import linkToLocation from '@mapbox/link-to-location';
 import querySelectorContainsNode from '@mapbox/query-selector-contains-node';
 import { batfishContext } from 'batfish-internal/context';
 import { routeTo } from '@mapbox/batfish/modules/route-to';
+import { prefixUrl } from '@mapbox/batfish/modules/prefix-url';
 import { findMatchingRoute } from './find-matching-route';
 import { scrollToFragment } from './scroll-to-fragment';
 import { getWindow } from './get-window';
 import { changePage } from './change-page';
 import { getCurrentLocation } from './get-current-location';
 
+// See explanation for this weirdness in prefix-url.js.
+// This happens outside the component lifecycle so it can be used during
+// rendering of HTML.
+prefixUrl._configure(
+  batfishContext.selectedConfig.siteBasePath,
+  batfishContext.selectedConfig.siteOrigin
+);
+
 type Props = {
   startingPath: string,
   startingComponent: React$ComponentType<*>,


Test patch:
diff --git a/test/prefix-url.test.js b/test/prefix-url.test.js
index 2f72956..1a5cd86 100644
--- a/test/prefix-url.test.js
+++ b/test/prefix-url.test.js
@@ -1,29 +1,16 @@
 'use strict';
 
-// eslint-disable-next-line node/no-missing-require
-const batfishContextModule = require('batfish-internal/context');
 const prefixUrl = require('../src/webpack/public/prefix-url').prefixUrl;
 const prefixUrlAbsolute = require('../src/webpack/public/prefix-url')
   .prefixUrlAbsolute;
 
-jest.mock(
-  'batfish-internal/context',
-  () => {
-    return {
-      batfishContext: {}
-    };
-  },
-  { virtual: true }
-);
-
 describe('prefixUrl', () => {
   beforeEach(() => {
-    batfishContextModule.batfishContext = {
-      selectedConfig: {
-        siteBasePath: '',
-        siteOrigin: ''
-      }
-    };
+    prefixUrl._configure();
+  });
+
+  afterEach(() => {
+    prefixUrl._configure();
   });
 
   test('prefixUrl without starting slash', () => {
@@ -39,14 +26,13 @@ describe('prefixUrl', () => {
   });
 });
 
-describe('createPrefixUrl generated module with siteBasePath and siteOrigin', () => {
+describe('_configure provides prefixUrl with siteBasePath and siteOrigin', () => {
   beforeEach(() => {
-    batfishContextModule.batfishContext = {
-      selectedConfig: {
-        siteBasePath: '/foo/bar',
-        siteOrigin: 'https://www.test.com'
-      }
-    };
+    prefixUrl._configure('/foo/bar', 'https://www.test.com');
+  });
+
+  afterEach(() => {
+    prefixUrl._configure();
   });
 
   test('prefixUrl without starting slash', () => {
diff --git a/test/router-prefix-url.test.js b/test/router-prefix-url.test.js
new file mode 100644
index 0000000..b5d0fd5
--- /dev/null
+++ b/test/router-prefix-url.test.js
@@ -0,0 +1,45 @@
+'use strict';
+
+// eslint-disable-next-line node/no-missing-require
+const prefixUrl = require('@mapbox/batfish/modules/prefix-url').prefixUrl;
+
+jest.mock(
+  '@mapbox/batfish/modules/prefix-url',
+  () => {
+    const prefixUrl = jest.fn();
+    prefixUrl._configure = jest.fn();
+    return { prefixUrl };
+  },
+  { virtual: true }
+);
+
+jest.mock(
+  'batfish-internal/context',
+  () => {
+    return {
+      batfishContext: {
+        selectedConfig: {
+          siteBasePath: 'mockSiteBasePath',
+          siteOrigin: 'mockSiteOrigin',
+          hijackLinks: true
+        }
+      }
+    };
+  },
+  { virtual: true }
+);
+
+jest.mock('@mapbox/batfish/modules/route-to', () => {}, { virtual: true });
+jest.mock('@mapbox/batfish/modules/route-change-listeners', () => {}, {
+  virtual: true
+});
+
+test('when Router is required, it configures prefixUrl', () => {
+  expect(prefixUrl._configure).toHaveBeenCalledTimes(0);
+  require('../src/webpack/router');
+  expect(prefixUrl._configure).toHaveBeenCalledTimes(1);
+  expect(prefixUrl._configure).toHaveBeenCalledWith(
+    'mockSiteBasePath',
+    'mockSiteOrigin'
+  );
+});