Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
chore: remove installAsyncStackHooks helper (#6186)
* chore: remove `installAsyncStackHooks` helper

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 :)

* lazy launcher is private

* remove async stack test
  • Loading branch information
jackfranklin committed Jul 9, 2020
1 parent 19f188a commit 1243466
Show file tree
Hide file tree
Showing 8 changed files with 8 additions and 88 deletions.
11 changes: 0 additions & 11 deletions new-docs/puppeteer.puppeteer._lazylauncher.md

This file was deleted.

1 change: 0 additions & 1 deletion new-docs/puppeteer.puppeteer.md
Expand Up @@ -40,7 +40,6 @@ const puppeteer = require('puppeteer');
| [\_\_productName](./puppeteer.puppeteer.__productname.md) | | string | |
| [\_changedProduct](./puppeteer.puppeteer._changedproduct.md) | | boolean | |
| [\_isPuppeteerCore](./puppeteer.puppeteer._ispuppeteercore.md) | | boolean | |
| [\_lazyLauncher](./puppeteer.puppeteer._lazylauncher.md) | | [ProductLauncher](./puppeteer.productlauncher.md) | |
| [\_preferredRevision](./puppeteer.puppeteer._preferredrevision.md) | | string | |
| [devices](./puppeteer.puppeteer.devices.md) | | [DevicesMap](./puppeteer.devicesmap.md) | |
| [errors](./puppeteer.puppeteer.errors.md) | | [PuppeteerErrors](./puppeteer.puppeteererrors.md) | |
Expand Down
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
15 changes: 0 additions & 15 deletions test/page.spec.ts
Expand Up @@ -119,21 +119,6 @@ describe('Page', function () {
});
});

describeFailsFirefox('Async stacks', () => {
it('should work', async () => {
const { page, server } = getTestState();

server.setRoute('/empty.html', (req, res) => {
res.statusCode = 204;
res.end();
});
let error = null;
await page.goto(server.EMPTY_PAGE).catch((error_) => (error = error_));
expect(error).not.toBe(null);
expect(error.stack).toContain(__filename);
});
});

// This test fails on Firefox on CI consistently but cannot be replicated
// locally. Skipping for now to unblock the Mitt release and given FF support
// isn't fully done yet but raising an issue to ask the FF folks to have a
Expand Down

0 comments on commit 1243466

Please sign in to comment.