Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reuse JSDOM instances across targets #164

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Reuse JSDOM instances across targets #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48cf902
Reuse JSDOM instances across targets
lencioni Jun 18, 2020
114c08f
Resolve remaining issues with a reused jsdom
trotzig Jun 18, 2020
853e4fd
Reset cursor in processor constructor
trotzig Jun 18, 2020
c3fa907
5.7.0-rc.1
trotzig Jun 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "happo.io",
"version": "5.6.1",
"version": "5.7.0-rc.1",
"description": "Visual diffing for UI components",
"main": "./build/index.js",
"bin": {
Expand Down
66 changes: 50 additions & 16 deletions src/JSDOMDomProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,28 @@
import { JSDOM, VirtualConsole } from 'jsdom';

import Logger from './Logger';

const { VERBOSE } = process.env;

const MAX_ERROR_DETAIL_LENGTH = 200;

export default class JSDOMDomProvider {
constructor(jsdomOptions, { width, height, webpackBundle }) {
// Cache the JSDOM instance because re-loading the webpack bundle for every
// target can be very expensive. This assumes that jsdomOptions and
// webpackBundle do not change.
let dom;
function getCachedDOM(jsdomOptions, webpackBundle) {
if (!dom) {
const logger = new Logger();
logger.start('Initializing JSDOM with the bundle...');

const virtualConsole = new VirtualConsole();
virtualConsole.on('jsdomError', (e) => {
const { stack, detail = '' } = e;
if (VERBOSE || typeof detail !== 'string' || detail.length < MAX_ERROR_DETAIL_LENGTH) {
if (
VERBOSE ||
typeof detail !== 'string' ||
detail.length < MAX_ERROR_DETAIL_LENGTH
) {
console.error(stack, detail);
} else {
const newDetail = `${(detail || '').slice(0, MAX_ERROR_DETAIL_LENGTH)}...
Expand All @@ -19,7 +32,7 @@ export default class JSDOMDomProvider {
});
virtualConsole.sendTo(console, { omitJSDOMErrors: true });

this.dom = new JSDOM(
dom = new JSDOM(
`
<!DOCTYPE html>
<html>
Expand All @@ -37,30 +50,51 @@ export default class JSDOMDomProvider {
url: 'http://localhost',
virtualConsole,
beforeParse(win) {
win.outerWidth = win.innerWidth = width;
win.outerHeight = win.innerHeight = height;
Object.defineProperties(win.screen, {
width: { value: width },
availWidth: { value: width },
height: { value: height },
availHeight: { value: height },
});
win.requestAnimationFrame = (callback) => setTimeout(callback, 0);
win.cancelAnimationFrame = clearTimeout;
},
},
jsdomOptions,
),
);

logger.success();
}

return dom;
}

// Useful for tests
export function clearCachedDOM() {
dom = undefined;
}

export default class JSDOMDomProvider {
constructor(jsdomOptions, { webpackBundle }) {
this.dom = getCachedDOM(jsdomOptions, webpackBundle);
}

async init({ targetName }) {
await new Promise((resolve) => {
this.dom.window.onBundleReady = resolve;
});
if (!this.dom.window.happoProcessor) {
await new Promise((resolve) => {
this.dom.window.onBundleReady = resolve;
});
}
return this.dom.window.happoProcessor.init({ targetName });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think inside the happoProcessor (which is src/browser/processor.js) we need to change some things so that the cursor is reset (and possibly other things as well).

}

resize({ width, height }) {
this.dom.window.outerWidth = this.dom.window.innerWidth = width;
this.dom.window.outerHeight = this.dom.window.innerHeight = height;
Object.defineProperties(this.dom.window.screen, {
width: { value: width, configurable: true },
availWidth: { value: width, configurable: true },
height: { value: height, configurable: true },
availHeight: { value: height, configurable: true },
});
return this.dom.window.happoProcessor.reset();
}

next() {
return this.dom.window.happoProcessor.next();
}
Expand All @@ -74,6 +108,6 @@ export default class JSDOMDomProvider {
}

close() {
this.dom.window.close();
// no-op
}
}
4 changes: 4 additions & 0 deletions src/browser/processor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ export default class Processor {
);
}

reset() {
this.cursor = -1;
}

addExamples(examples) {
examples.forEach(({ fileName, component, variants }) => {
Object.keys(variants).forEach((variant) => {
Expand Down
15 changes: 10 additions & 5 deletions src/processSnapsInBundle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,23 @@ export default async function processSnapsInBundle(
{ viewport, DomProvider, targetName },
) {
const [width, height] = viewport.split('x').map((s) => parseInt(s, 10));
const domProvider = new DomProvider({
webpackBundle,
width,
height,
});

// TODO Remove width and height in next breaking change after puppeteer plugin
// has been updated.
const domProvider = new DomProvider({ webpackBundle, width, height });

const result = {
snapPayloads: [],
};
try {
await domProvider.init({ targetName });

// TODO remove resize guard in next breaking change and after puppeteer
// plugin has been updated.
if (typeof domProvider.resize !== 'undefined') {
await domProvider.resize({ width, height });
}

// Disabling eslint here because we actually want to run things serially.
/* eslint-disable no-await-in-loop */
while (await domProvider.next()) {
Expand Down
2 changes: 2 additions & 0 deletions test/integrations/error-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import MockTarget from './MockTarget';
import * as defaultConfig from '../../src/DEFAULTS';
import makeRequest from '../../src/makeRequest';
import runCommand from '../../src/commands/run';
import { clearCachedDOM } from '../../src/JSDOMDomProvider';

jest.mock('../../src/makeRequest');

Expand All @@ -15,6 +16,7 @@ let config;
let sha;

beforeEach(() => {
clearCachedDOM();
console.warn = jest.fn(originalConsoleWarn);
console.error = jest.fn(originalConsoleErr);
console.error.mockReset();
Expand Down
3 changes: 3 additions & 0 deletions test/integrations/react-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import MockTarget from './MockTarget';
import * as defaultConfig from '../../src/DEFAULTS';
import makeRequest from '../../src/makeRequest';
import runCommand from '../../src/commands/run';
import { clearCachedDOM } from '../../src/JSDOMDomProvider';

jest.mock('../../src/makeRequest');

Expand All @@ -16,6 +17,8 @@ let config;
let sha;

beforeEach(() => {
clearCachedDOM();

makeRequest.mockImplementation(() => Promise.resolve({}));
sha = 'foobar';
config = Object.assign({}, defaultConfig, {
Expand Down