Extracted page scraping loop from react component to separate module, and added unit tests (#18)

DrewML · web-flow · commit f585ce7497fd · 2018-12-04T15:45:51.000-06:00
diff --git a/jest.config.js b/jest.config.js
@@ -5,7 +5,7 @@
 
 module.exports = {
     preset: 'ts-jest',
-    testEnvironment: 'node',
+    testEnvironment: 'jsdom',
     setupFiles: [
         // v8 with built-in flat (v6.9.x) won't be in node until v11.
         // Do not remove this polyfill until node 12 is LTS
diff --git a/src/App/index.tsx b/src/App/index.tsx
@@ -60,6 +60,7 @@ export default class App extends React.Component<{}, State> {
                         // each tab to persist its state without having to hoist
                         // state to the root of the tree
                         <div
+                            key={panel.title}
                             className={styles.rightPanelItemWrapper}
                             data-selected={panel === selectedPanel}
                         >
diff --git a/src/RequireJS/BundleGenerator/ConfigGen.tsx b/src/RequireJS/BundleGenerator/ConfigGen.tsx
@@ -4,18 +4,18 @@
  */
 
 import * as React from 'react';
-import { RequireConfig, ModulesByPageType } from '../../types/require';
+import { RequireConfig, PageModule } from '../../types/require';
 import generate from './generate';
 
 type Props = {
-    modulesByPageType: ModulesByPageType;
+    pageModules: PageModule[];
     requireConfig: RequireConfig;
 };
 
 export default class ConfigGen extends React.Component<Props> {
     render() {
-        const { modulesByPageType, requireConfig } = this.props;
-        const config = generate(modulesByPageType, requireConfig);
+        const { pageModules, requireConfig } = this.props;
+        const config = generate(pageModules, requireConfig);
 
         return <pre>{JSON.stringify(config, null, 2)}</pre>;
     }
diff --git a/src/RequireJS/BundleGenerator/PageCollector.ts b/src/RequireJS/BundleGenerator/PageCollector.ts
@@ -0,0 +1,70 @@
+/**
+ * Copyright © Magento, Inc. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+
+import { PageModule, RequireConfig } from '../../types/require';
+import { getBundlingData } from '../../interop';
+
+type CollectVal = {
+    config: RequireConfig;
+    page: PageModule;
+};
+type Callback = (data: CollectVal) => void;
+
+export default class PageCollector {
+    timerID: number | null = null;
+    private listeners: Set<Callback> = new Set();
+
+    private pollForModules() {
+        this.timerID = window.setTimeout(async () => {
+            try {
+                const {
+                    modules,
+                    config,
+                    pageConfigType,
+                    url,
+                } = await getBundlingData();
+
+                // TODO: Actually handle this case, since it will point to a bug
+                // in the shaky logic of `_getPageConfigType` in `interop.ts`
+                if (!pageConfigType) throw new Error('No pageConfigType found');
+
+                const page = {
+                    url,
+                    pageConfigType,
+                    // TODO: Address this hack better. m2 has a bug where
+                    // a superfluous mixin is added to the Require registry for any
+                    // require() with a relative URL (./).
+                    modules: modules.filter(m => !m.startsWith('mixins!')),
+                };
+
+                this.notifyListeners({ page, config });
+            } catch (err) {
+                // TODO: Handle errors. Most common error is us attempting
+                // to eval code in the currently-inspected page when it's
+                // between pages, which can be safely ignored
+            }
+
+            this.pollForModules();
+        }, 200);
+    }
+
+    private notifyListeners(val: CollectVal) {
+        this.listeners.forEach(f => f(val));
+    }
+
+    private stopPolling() {
+        if (this.timerID) window.clearTimeout(this.timerID);
+    }
+
+    unsubscribe(cb: Callback) {
+        this.listeners.delete(cb);
+        if (!this.listeners.size) this.stopPolling();
+    }
+
+    subscribe(cb: Callback) {
+        this.listeners.add(cb);
+        if (this.listeners.size === 1) this.pollForModules();
+    }
+}
diff --git a/src/RequireJS/BundleGenerator/RecordingProgress.tsx b/src/RequireJS/BundleGenerator/RecordingProgress.tsx
@@ -4,21 +4,21 @@
  */
 
 import * as React from 'react';
-import { ModulesByPageType, PageModule } from '../../types/require';
+import { ModulesByURL, PageModule } from '../../types/require';
 import styles from './RecordingProgress.css';
 
 type Props = {
-    modulesByPageType: ModulesByPageType;
+    modulesByURL: ModulesByURL;
 };
 
 export default class RecordingProgress extends React.Component<Props> {
     render() {
-        const { modulesByPageType } = this.props;
+        const { modulesByURL } = this.props;
 
         return (
             <div className={styles.wrapper}>
                 <ul className={styles.pageList}>
-                    {modulesByPageType.map(mod => (
+                    {Object.values(modulesByURL).map(mod => (
                         <PageTypeTile key={mod.url} mod={mod} />
                     ))}
                 </ul>
diff --git a/src/RequireJS/BundleGenerator/__tests__/PageCollector.test.ts b/src/RequireJS/BundleGenerator/__tests__/PageCollector.test.ts
@@ -0,0 +1,67 @@
+/**
+ * Copyright © Magento, Inc. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+
+import PageCollector from '../PageCollector';
+import { getBundlingData } from '../../../interop';
+
+jest.mock('../../../interop');
+jest.useFakeTimers();
+
+beforeEach(() => {
+    jest.clearAllMocks();
+    jest.clearAllTimers();
+});
+
+test('Does not start polling until a subscription exists', () => {
+    const collector = new PageCollector();
+    jest.runAllTimers();
+    expect(getBundlingData).not.toHaveBeenCalled();
+
+    const subscriber = jest.fn();
+    collector.subscribe(subscriber);
+    jest.runAllTimers();
+
+    expect(getBundlingData).toHaveBeenCalled();
+});
+
+test('Stops notifying subscriber when it is unsubscribed', () => {
+    const collector = new PageCollector();
+    const subscriber = jest.fn();
+    collector.subscribe(subscriber);
+
+    jest.runAllTimers();
+    const callCountBeforeUnsubscribe = subscriber.mock.calls.length;
+    collector.unsubscribe(subscriber);
+    jest.runAllTimers();
+
+    expect(subscriber).toHaveBeenCalledTimes(callCountBeforeUnsubscribe);
+});
+
+test('Removes modules that are prepended with "mixins!"', () => {
+    const mock = (getBundlingData as unknown) as jest.MockInstance<
+        typeof getBundlingData
+    >;
+    mock.mockReturnValue(
+        Promise.resolve({
+            url: 'https://www.site.com/product',
+            config: {},
+            pageConfigType: 'catalog-product-view',
+            modules: ['foo', 'bar', 'mixins!bar', 'bizz'],
+        }),
+    );
+
+    const collector = new PageCollector();
+    const subscriber = jest.fn();
+    collector.subscribe(subscriber);
+
+    jest.runAllTimers();
+
+    // Unlike tests using the `jest.mock` automock functionality,
+    // we need to wait briefly here because the Promise passed to `mock.mockReturnValue`
+    // above uses a microtask to unwrap
+    setImmediate(() => {
+        expect(subscriber).toHaveBeenCalledTimes(1);
+    });
+});
diff --git a/src/RequireJS/BundleGenerator/generate.ts b/src/RequireJS/BundleGenerator/generate.ts
@@ -3,11 +3,7 @@
  * See COPYING.txt for license details.
  */
 
-import {
-    ModulesByPageType,
-    ShimConfig,
-    RequireConfig,
-} from '../../types/require';
+import { ShimConfig, RequireConfig, PageModule } from '../../types/require';
 import intersection from 'lodash.intersection';
 import difference from 'lodash.difference';
 
@@ -35,13 +31,11 @@ type BundleConfig = {
 };
 
 export default function generate(
-    modulesByPageType: ModulesByPageType,
+    pageModules: PageModule[],
     requireConfig: RequireConfig,
 ): BundleConfig {
     // Cheap clone so we can feel free to mutate
-    const modules = JSON.parse(
-        JSON.stringify(modulesByPageType),
-    ) as ModulesByPageType;
+    const modules = JSON.parse(JSON.stringify(pageModules)) as PageModule[];
 
     const commons: string[] = [];
     modules.forEach(mod => {
diff --git a/src/RequireJS/BundleGenerator/index.tsx b/src/RequireJS/BundleGenerator/index.tsx
@@ -8,119 +8,68 @@ import Toolbar from './Toolbar';
 import ConfigGen from './ConfigGen';
 import styles from './BundleGenerator.css';
 import c from '../../classjoin';
-import {
-    getLoadedModules,
-    getRequireConfig,
-    getPageConfigType,
-    getURL,
-} from '../../interop';
-import { ModulesByPageType, RequireConfig } from '../../types/require';
+import { RequireConfig, PageModule } from '../../types/require';
 import RecordingProgress from './RecordingProgress';
+import PageCollector from './PageCollector';
 
 type States = 'RECORDING' | 'RESULTS' | 'WELCOME';
 type State = {
     currentState: States;
-    modulesByPageType: ModulesByPageType;
+    modulesByURL: { [key: string]: PageModule };
     requireConfig: RequireConfig | null;
 };
 
-// TODO:
-// - Merge modules from multiple URLs with same layout handle, instead of taking the latest
-//      - Track URLs, leave comments in `r.js` generated config
-// - Allow purging records for a visited URL. IOW, if you've visited 4 product pages, but
-//   want to purge the results from 1 of them
-// - Support a user provided list of URLs to drive the browser too, instead of manual nav
-//   for frequent users
 export default class BundleGenerator extends React.Component<{}, State> {
     state: State = {
-        modulesByPageType: [],
+        modulesByURL: {},
         currentState: 'WELCOME',
         requireConfig: null,
     };
-    timerID: number | null = null;
+    collector: PageCollector = new PageCollector();
 
     onRecord = () => {
         this.setState(({ currentState }) => {
-            const isStop = currentState === 'RECORDING';
-            isStop ? this.stopPolling() : this.pollForModules();
+            const shouldStopRecording = currentState === 'RECORDING';
+            shouldStopRecording ? this.stopPolling() : this.pollForModules();
             return {
-                currentState: isStop ? 'RESULTS' : 'RECORDING',
+                currentState: shouldStopRecording ? 'RESULTS' : 'RECORDING',
             };
         });
     };
 
     onClear = () => {
         this.setState({
             currentState: 'WELCOME',
-            modulesByPageType: [],
+            modulesByURL: {},
             requireConfig: null,
         });
     };
 
     stopPolling() {
-        this.timerID && window.clearTimeout(this.timerID);
+        this.collector.unsubscribe(this.collectorCallback);
     }
 
-    // TODO: Abstract the body of this method outside of the component
-    // and test it. Also clean it up because it's a mess
-    pollForModules() {
-        this.timerID = window.setTimeout(async () => {
-            try {
-                const [
-                    modules,
-                    requireConfig,
-                    pageConfigType,
-                    url,
-                ] = await Promise.all([
-                    getLoadedModules(),
-                    getRequireConfig(),
-                    getPageConfigType(),
-                    getURL(),
-                ]);
-
-                // TODO: Actually handle this case, since it will point to a bug
-                // in the shaky logic of `_getPageConfigType` in `interop.ts`
-                if (!pageConfigType) throw new Error('No pageConfigType found');
-
-                this.setState(({ modulesByPageType }) => {
-                    const [current] = modulesByPageType.filter(
-                        m => m.pageConfigType === pageConfigType,
-                    );
-                    const mod = {
-                        url,
-                        pageConfigType,
-                        // TODO: Address this hack better. m2 has a bug where
-                        // a superfluous mixin is added to the Require registry for any
-                        // require() with a relative URL (./).
-                        modules: modules.filter(m => !m.startsWith('mixins!')),
-                    };
-                    current
-                        ? modulesByPageType.splice(
-                              modulesByPageType.indexOf(current),
-                              1,
-                              mod,
-                          )
-                        : modulesByPageType.push(mod);
-
-                    return {
-                        requireConfig,
-                        modulesByPageType,
-                    };
-                });
-            } catch (err) {
-                console.error('Failure in pollForModules()', err);
-            }
+    collectorCallback = (data: { page: PageModule; config: RequireConfig }) => {
+        const { page, config } = data;
+        this.setState(prevState => ({
+            modulesByURL: {
+                ...prevState.modulesByURL,
+                [page.url]: page,
+            },
+            requireConfig: config,
+        }));
+    };
 
-            this.pollForModules();
-        }, 200);
+    pollForModules() {
+        this.collector.subscribe(this.collectorCallback);
     }
 
     componentWillUnmount() {
         this.stopPolling();
     }
 
     render() {
-        const { currentState, modulesByPageType, requireConfig } = this.state;
+        const { currentState, modulesByURL, requireConfig } = this.state;
 
         return (
             <>
@@ -131,11 +80,11 @@ export default class BundleGenerator extends React.Component<{}, State> {
                 />
                 {currentState === 'WELCOME' && welcomeContent}
                 {currentState === 'RECORDING' && (
-                    <RecordingProgress modulesByPageType={modulesByPageType} />
+                    <RecordingProgress modulesByURL={modulesByURL} />
                 )}
                 {currentState === 'RESULTS' && requireConfig && (
                     <ConfigGen
-                        modulesByPageType={modulesByPageType}
+                        pageModules={Object.values(modulesByURL)}
                         requireConfig={requireConfig}
                     />
                 )}
diff --git a/src/RequireJS/index.tsx b/src/RequireJS/index.tsx
@@ -46,6 +46,7 @@ export default class RequireJS extends React.Component<{}, State> {
                 />
                 {tabs.map(tab => (
                     <div
+                        key={tab.name}
                         className={styles.tabWrapper}
                         data-selected={tab === selectedTab}
                     >
diff --git a/src/interop.ts b/src/interop.ts
diff --git a/src/types/require.ts b/src/types/require.ts

Original file line number	Diff line number	Diff line change
`@@ -60,6 +60,7 @@ export default class App extends React.Component<{}, State> {`
`60`	`60`	`// each tab to persist its state without having to hoist`
`61`	`61`	`// state to the root of the tree`
`62`	`62`	`<div`
	`63`	`+ key={panel.title}`
`63`	`64`	`className={styles.rightPanelItemWrapper}`
`64`	`65`	`data-selected={panel === selectedPanel}`
`65`	`66`	`>`
Original file line number	Diff line number	Diff line change
`@@ -46,6 +46,7 @@ export default class RequireJS extends React.Component<{}, State> {`
`46`	`46`	`/>`
`47`	`47`	`{tabs.map(tab => (`
`48`	`48`	`<div`
	`49`	`+ key={tab.name}`
`49`	`50`	`className={styles.tabWrapper}`
`50`	`51`	`data-selected={tab === selectedTab}`
`51`	`52`	`>`