Skip to content

Commit

Permalink
chore: run unit tests on node 10.15 + fix fs.promises access (puppete…
Browse files Browse the repository at this point in the history
…er#6550)

* chore: run unit tests on node 10.15

We saw in puppeteer#6548 that the
`fs.promises` module was experimental in Node <10.17 and as such we
introduced issues for users on 10.15.

Until we can drop Node v10 (it's EOL is 30-04-20201
https://github.com/nodejs/Release#release-schedule) we should run our
tests on an old Node 10 to avoid regressing in this area.

* chore: helper for importing fs safely
  • Loading branch information
jackfranklin authored Oct 26, 2020
1 parent a2175c6 commit e45acce
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 7 deletions.
15 changes: 14 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,20 @@ jobs:
- npm run tsc
- travis_retry npm run unit

# Runs unit tests on Linux + Chromium
# Node <10.17's fs.promises module was experimental and doesn't behave as
# expected. This problem was fixed in Node 10.19, but we run the unit tests
# through on 10.15 to make sure we don't cause any regressions when using
# fs.promises. See https://github.com/puppeteer/puppeteer/issues/6548 for an
# example.
- node_js: '10.15.0'
name: 'Node 10.15 Unit tests: Linux/Chromium'
env:
- CHROMIUM=true
before_install:
- PUPPETEER_PRODUCT=firefox npm install
script:
- npm run unit

- node_js: '10.19.0'
name: 'Unit tests [with coverage]: Linux/Chromium'
env:
Expand Down
4 changes: 2 additions & 2 deletions src/common/DOMWorld.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ export class DOMWorld {
'Cannot pass a filepath to addScriptTag in the browser environment.'
);
}
const fs = await import('fs');
const fs = await helper.importFSModule();
let contents = await fs.promises.readFile(path, 'utf8');
contents += '//# sourceURL=' + path.replace(/\n/g, '');
const context = await this.executionContext();
Expand Down Expand Up @@ -370,7 +370,7 @@ export class DOMWorld {
'Cannot pass a filepath to addStyleTag in the browser environment.'
);
}
const fs = await import('fs');
const fs = await helper.importFSModule();
let contents = await fs.promises.readFile(path, 'utf8');
contents += '/*# sourceURL=' + path.replace(/\n/g, '') + '*/';
const context = await this.executionContext();
Expand Down
3 changes: 1 addition & 2 deletions src/common/JSHandle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -563,8 +563,7 @@ export class ElementHandle<
// 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 fs = await import('fs');
const fs = await helper.importFSModule();
// Locate all files and confirm that they exist.
const files = await Promise.all(
filePaths.map(async (filePath) => {
Expand Down
2 changes: 1 addition & 1 deletion src/common/Page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1709,7 +1709,7 @@ export class Page extends EventEmitter {
'Screenshots can only be written to a file path in a Node environment.'
);
}
const fs = await import('fs');
const fs = await helper.importFSModule();
if (options.path) await fs.promises.writeFile(options.path, buffer);
return buffer;

Expand Down
27 changes: 26 additions & 1 deletion src/common/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ async function readProtocolStream(
throw new Error('Cannot write to a path outside of Node.js environment.');
}

const fs = isNode ? await import('fs') : null;
const fs = isNode ? await importFSModule() : null;

let eof = false;
let fileHandle: import('fs').promises.FileHandle;
Expand Down Expand Up @@ -344,6 +344,30 @@ async function readProtocolStream(
}
}

/**
* Loads the Node fs promises API. Needed because on Node 10.17 and below,
* fs.promises is experimental, and therefore not marked as enumerable. That
* means when TypeScript compiles an `import('fs')`, its helper doesn't spot the
* promises declaration and therefore on Node <10.17 you get an error as
* fs.promises is undefined in compiled TypeScript land.
*
* See https://github.com/puppeteer/puppeteer/issues/6548 for more details.
*
* Once Node 10 is no longer supported (April 2021) we can remove this and use
* `(await import('fs')).promises`.
*/
async function importFSModule(): Promise<typeof import('fs')> {
if (!isNode) {
throw new Error('Cannot load the fs module API outside of Node.');
}

const fs = await import('fs');
if (fs.promises) {
return fs;
}
return fs.default;
}

export const helper = {
evaluationString,
pageBindingInitString,
Expand All @@ -356,6 +380,7 @@ export const helper = {
waitForEvent,
isString,
isNumber,
importFSModule,
addEventListener,
removeEventListeners,
valueFromRemoteObject,
Expand Down

0 comments on commit e45acce

Please sign in to comment.