Skip to content

Commit

Permalink
chore(agnostic): Remove use of util.promisify (puppeteer#6446)
Browse files Browse the repository at this point in the history
In `src/common` we now use `fs.promises.X` which we can dynamically
`import`. In a browser environment this code will never run because it's
gated on `isNode` (in a future PR we will add tree-shaking to the bundle
step such that this code is eliminated). By using `import`, we ensure
TypeScript still can track types and give good type information.

In `src/node` we continue to use `util.promisify` but that's not a
concern as that code explicitly is never run in the browser.
  • Loading branch information
jackfranklin authored Sep 28, 2020
1 parent 96f3d43 commit caa9a1c
Show file tree
Hide file tree
Showing 8 changed files with 38 additions and 35 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
"generate-docs": "npm run tsc && api-extractor run --local --verbose && api-documenter markdown -i temp -o new-docs",
"ensure-new-docs-up-to-date": "npm run generate-docs && git status && exit `git status --porcelain | head -255 | wc -l`",
"generate-dependency-graph": "echo 'Requires graphviz installed locally!' && depcruise --exclude 'api.ts' --do-not-follow '^node_modules' --output-type dot src/index.ts | dot -T png > dependency-chart.png",
"ensure-correct-devtools-protocol-revision": "ts-node scripts/ensure-correct-devtools-protocol-package"
"ensure-correct-devtools-protocol-revision": "ts-node -s scripts/ensure-correct-devtools-protocol-package"
},
"files": [
"lib/",
Expand Down
6 changes: 6 additions & 0 deletions scripts/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"extends": "../tsconfig.json",
"compilerOptions": {
"module": "CommonJS"
}
}
16 changes: 4 additions & 12 deletions src/common/DOMWorld.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,12 +275,8 @@ export class DOMWorld {
'Cannot pass a filepath to addScriptTag in the browser environment.'
);
}
// eslint-disable-next-line @typescript-eslint/no-var-requires
const fs = require('fs');
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { promisify } = require('util');
const readFileAsync = promisify(fs.readFile);
let contents = await readFileAsync(path, 'utf8');
const fs = await import('fs');
let contents = await fs.promises.readFile(path, 'utf8');
contents += '//# sourceURL=' + path.replace(/\n/g, '');
const context = await this.executionContext();
return (
Expand Down Expand Up @@ -361,12 +357,8 @@ export class DOMWorld {
'Cannot pass a filepath to addStyleTag in the browser environment.'
);
}
// eslint-disable-next-line @typescript-eslint/no-var-requires
const fs = require('fs');
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { promisify } = require('util');
const readFileAsync = promisify(fs.readFile);
let contents = await readFileAsync(path, 'utf8');
const fs = await import('fs');
let contents = await fs.promises.readFile(path, 'utf8');
contents += '/*# sourceURL=' + path.replace(/\n/g, '') + '*/';
const context = await this.executionContext();
return (
Expand Down
17 changes: 9 additions & 8 deletions src/common/JSHandle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
WrapElementHandle,
UnwrapPromiseLike,
} from './EvalTypes.js';
import { isNode } from '../environment.js';

export interface BoxModel {
content: Array<{ x: number; y: number }>;
Expand Down Expand Up @@ -556,22 +557,22 @@ export class ElementHandle<
'Multiple file uploads only work with <input type=file multiple>'
);

if (!isNode) {
throw new Error(
`JSHandle#uploadFile can only be used in Node environments.`
);
}
// This import is only needed for `uploadFile`, so keep it scoped here to avoid paying
// the cost unnecessarily.
const path = await import('path');
// eslint-disable-next-line @typescript-eslint/no-var-requires
const path = require('path');
// eslint-disable-next-line @typescript-eslint/no-var-requires
const fs = require('fs');
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { promisify } = require('util');
const access = promisify(fs.access);

const fs = await import('fs');
// Locate all files and confirm that they exist.
const files = await Promise.all(
filePaths.map(async (filePath) => {
const resolvedPath: string = path.resolve(filePath);
try {
await access(resolvedPath, fs.constants.R_OK);
await fs.promises.access(resolvedPath, fs.constants.R_OK);
} catch (error) {
if (error.code === 'ENOENT')
throw new Error(`${filePath} does not exist or is not readable`);
Expand Down
13 changes: 8 additions & 5 deletions src/common/Page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
* limitations under the License.
*/

import * as fs from 'fs';
import { promisify } from 'util';
import { EventEmitter } from './EventEmitter.js';
import {
Connection,
Expand Down Expand Up @@ -57,8 +55,7 @@ import {
UnwrapPromiseLike,
} from './EvalTypes.js';
import { PDFOptions, paperFormats } from './PDFOptions.js';

const writeFileAsync = promisify(fs.writeFile);
import { isNode } from '../environment.js';

/**
* @public
Expand Down Expand Up @@ -1750,7 +1747,13 @@ export class Page extends EventEmitter {
options.encoding === 'base64'
? result.data
: Buffer.from(result.data, 'base64');
if (options.path) await writeFileAsync(options.path, buffer);
if (!isNode && options.path) {
throw new Error(
'Screenshots can only be written to a file path in a Node environment.'
);
}
const fs = await import('fs');
if (options.path) await fs.promises.writeFile(options.path, buffer);
return buffer;

function processClip(
Expand Down
15 changes: 6 additions & 9 deletions src/common/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,10 @@ import { TimeoutError } from './Errors.js';
import { debug } from './Debug.js';
import * as fs from 'fs';
import { CDPSession } from './Connection.js';
import { promisify } from 'util';
import { Protocol } from 'devtools-protocol';
import { CommonEventEmitter } from './EventEmitter.js';
import { assert } from './assert.js';

const openAsync = promisify(fs.open);
const writeAsync = promisify(fs.write);
const closeAsync = promisify(fs.close);

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

function getExceptionMessage(
Expand Down Expand Up @@ -207,8 +202,10 @@ async function readProtocolStream(
path?: string
): Promise<Buffer> {
let eof = false;
let file;
if (path) file = await openAsync(path, 'w');
let fileHandle: fs.promises.FileHandle;
if (path) {
fileHandle = await fs.promises.open(path, 'w');
}
const bufs = [];
while (!eof) {
const response = await client.send('IO.read', { handle });
Expand All @@ -218,9 +215,9 @@ async function readProtocolStream(
response.base64Encoded ? 'base64' : undefined
);
bufs.push(buf);
if (path) await writeAsync(file, buf);
if (path) await fs.promises.writeFile(fileHandle, buf);
}
if (path) await closeAsync(file);
if (path) await fileHandle.close();
await client.send('IO.close', { handle });
let resultBuffer = null;
try {
Expand Down
3 changes: 3 additions & 0 deletions test/tsconfig.test.json
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
{
"extends": "../tsconfig.base.json",
"compilerOptions": {
"module": "CommonJS"
}
}
1 change: 1 addition & 0 deletions tsconfig.base.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"checkJs": true,
"target": "ESNext",
"moduleResolution": "node",
"module": "ESNext",
"declaration": true,
"declarationMap": true,
"resolveJsonModule": true
Expand Down

0 comments on commit caa9a1c

Please sign in to comment.