Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

chore(template): add pull request template #43

Merged
merged 11 commits into from
Mar 4, 2020
35 changes: 35 additions & 0 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<!--- Provide a general summary of your changes in the Title above -->

## Description
<!--- Describe your changes in detail -->

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, tests ran to see how -->
<!--- your change affects other areas of the code, etc. -->

## Types of Changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation (adding or updating documentation)

## Checklist:
JAdshead marked this conversation as resolved.
Show resolved Hide resolved
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ ] My change requires a change to the documentation and I have updated the documentation accordingly.
- [ ] These changes should be applied to a maintenance branch.
- [ ] This change requires cross browser checks.
- [ ] Performance tests should be ran against the server prior to merging.
- [ ] This change impacts caching for client browsers.
- [ ] This change impacts HTTP headers.
- [ ] This change adds additional environment variable requirements for One App users.
- [ ] I have added the Apache 2.0 license header to any new files created.

## What is the Impact to Developers Using One App?
<!--- Please describe how your changes impacts developers using One App. -->
2 changes: 0 additions & 2 deletions dangerfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,8 @@
* permissions and limitations under the License.
*/

import preflightChecklist from './scripts/dangers/preflight-checklist';
import checkPackageFiles from './scripts/dangers/keep-package-and-lock-in-sync';
import reportBundleSizes from './scripts/dangers/bundle-sizes';

preflightChecklist();
checkPackageFiles();
reportBundleSizes();
21 changes: 13 additions & 8 deletions scripts/dangers/bundle-sizes.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import fs from 'fs';
import { exec } from 'child_process';
import path from 'path';
import zlib from 'zlib';
import { Writable } from 'stream';
import { Writable, pipeline } from 'stream';
import { promisify } from 'util';

import glob from 'glob';
Expand Down Expand Up @@ -68,14 +68,19 @@ function getGzipSize(filePath) {
callback();
},
});

return new Promise((res, rej) => {
fs.createReadStream(filePath)
.pipe(zlib.createGzip({ level: 9 }))
// writable streams do not fire the 'close' event anymore
// https://github.com/nodejs/node/issues/21122#issuecomment-414622251
.on('close', () => res(bytesWritten))
.pipe(byteCounter)
.on('error', rej);
pipeline(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion actually came from James Snell. He is on the Node.js TSC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice name drop lol

fs.createReadStream(filePath),
zlib.createGzip({ level: 9 }),
byteCounter,
(err) => {
if (err) {
rej(err);
}
res(bytesWritten);
}
);
});
}

Expand Down
40 changes: 0 additions & 40 deletions scripts/dangers/preflight-checklist.js

This file was deleted.