Skip to content

Commit

Permalink
chore: remove installAsyncStackHooks helper
Browse files Browse the repository at this point in the history
This code was written when browsers/Node didn't support errors in async
functions very well. They now do a much better job of this, so we can
lose the additonal complexity from our codebase and leave it to the host
environment :)
  • Loading branch information
jackfranklin committed Jul 9, 2020
1 parent 19f188a commit 6d7a6be
Show file tree
Hide file tree
Showing 5 changed files with 8 additions and 61 deletions.
5 changes: 1 addition & 4 deletions src/api.ts
Expand Up @@ -13,13 +13,10 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
// @ts-nocheck

/* This file is used in two places:
/* This file is used in one place:
* 1) the coverage-utils use it to gain a list of all methods we check for test
* coverage on
* 2) index.js uses it to iterate through all methods and call
* helper.installAsyncStackHooks on
*/
module.exports = {
Accessibility: require('./common/Accessibility').Accessibility,
Expand Down
2 changes: 1 addition & 1 deletion src/common/Puppeteer.ts
Expand Up @@ -61,7 +61,7 @@ export class Puppeteer {
_isPuppeteerCore: boolean;
_changedProduct = false;
__productName: string;
_lazyLauncher: ProductLauncher;
private _lazyLauncher: ProductLauncher;

/**
* @internal
Expand Down
38 changes: 0 additions & 38 deletions src/common/helper.ts
Expand Up @@ -28,10 +28,6 @@ const closeAsync = promisify(fs.close);

export const debugError = debug('puppeteer:error');

interface AnyClass {
prototype: object;
}

function getExceptionMessage(
exceptionDetails: Protocol.Runtime.ExceptionDetails
): string {
Expand Down Expand Up @@ -95,39 +91,6 @@ async function releaseObject(
});
}

function installAsyncStackHooks(classType: AnyClass): void {
for (const methodName of Reflect.ownKeys(classType.prototype)) {
const method = Reflect.get(classType.prototype, methodName);
if (
methodName === 'constructor' ||
typeof methodName !== 'string' ||
methodName.startsWith('_') ||
typeof method !== 'function' ||
method.constructor.name !== 'AsyncFunction'
)
continue;
Reflect.set(classType.prototype, methodName, function (...args) {
const syncStack = {
stack: '',
};
Error.captureStackTrace(syncStack);
return method.call(this, ...args).catch((error) => {
const stack = syncStack.stack.substring(
syncStack.stack.indexOf('\n') + 1
);
const clientStack = stack.substring(stack.indexOf('\n'));
if (
error instanceof Error &&
error.stack &&
!error.stack.includes(clientStack)
)
error.stack += '\n -- ASYNC --\n' + stack;
throw error;
});
});
}
}

export interface PuppeteerEventListener {
emitter: CommonEventEmitter;
eventName: string | symbol;
Expand Down Expand Up @@ -277,7 +240,6 @@ export const helper = {
addEventListener,
removeEventListeners,
valueFromRemoteObject,
installAsyncStackHooks,
getExceptionMessage,
releaseObject,
};
19 changes: 1 addition & 18 deletions src/initialize.ts
Expand Up @@ -14,24 +14,13 @@
* limitations under the License.
*/

// api.ts has to use module.exports as it's also consumed by DocLint which runs
// on Node.
// eslint-disable-next-line @typescript-eslint/no-var-requires
const api = require('./api');

import { helper } from './common/helper';
import { Puppeteer } from './common/Puppeteer';
import { PUPPETEER_REVISIONS } from './revisions';
import pkgDir from 'pkg-dir';

export const initializePuppeteer = (packageName: string): Puppeteer => {
const puppeteerRootDirectory = pkgDir.sync(__dirname);

for (const className in api) {
if (typeof api[className] === 'function')
helper.installAsyncStackHooks(api[className]);
}

let preferredRevision = PUPPETEER_REVISIONS.chromium;
const isPuppeteerCore = packageName === 'puppeteer-core';
// puppeteer-core ignores environment variables
Expand All @@ -43,16 +32,10 @@ export const initializePuppeteer = (packageName: string): Puppeteer => {
if (!isPuppeteerCore && product === 'firefox')
preferredRevision = PUPPETEER_REVISIONS.firefox;

const puppeteer = new Puppeteer(
return new Puppeteer(
puppeteerRootDirectory,
preferredRevision,
isPuppeteerCore,
product
);

// The introspection in `Helper.installAsyncStackHooks` references
// `Puppeteer._launcher` before the Puppeteer ctor is called, such that an
// invalid Launcher is selected at import, so we reset it.
puppeteer._lazyLauncher = undefined;
return puppeteer;
};
5 changes: 5 additions & 0 deletions test/launcher.spec.ts
Expand Up @@ -441,6 +441,11 @@ describe('Launcher specs', function () {

after(async () => {
const { puppeteer } = getTestState();
/* launcher is a private property so we don't want our users doing this
* but we need to reset the state fully here for testing different
* browser launchers
*/
// @ts-expect-error
puppeteer._lazyLauncher = undefined;
puppeteer._productName = productName;
});
Expand Down

0 comments on commit 6d7a6be

Please sign in to comment.