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.

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

chore: remove installAsyncStackHooks helper #6186

Merged
merged 3 commits into from Jul 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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