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

add code of generating artifacts #5519

Merged
merged 8 commits into from Jul 19, 2018

Conversation

Projects
None yet
5 participants
@Claudiazhaoya
Collaborator

Claudiazhaoya commented Jul 11, 2018

aim to generate build artifacts once build finishes .

Microsoft Reviewers: Open in CodeFlow

@Claudiazhaoya Claudiazhaoya requested a review from dzearing as a code owner Jul 11, 2018

Show outdated Hide outdated scripts/build.js Outdated
Show outdated Hide outdated scripts/build.js Outdated
Show outdated Hide outdated scripts/build.js Outdated

Claudiazhaoya added some commits Jul 13, 2018

@Claudiazhaoya

This comment has been minimized.

Show comment
Hide comment
@Claudiazhaoya

Claudiazhaoya Jul 13, 2018

Collaborator

add conditions to generate build artifacts

Collaborator

Claudiazhaoya commented Jul 13, 2018

add conditions to generate build artifacts

Show outdated Hide outdated scripts/tasks/size-audit.js Outdated
Show outdated Hide outdated scripts/tasks/size-audit.js Outdated
}
}
function getFileSize(filePath) {

This comment has been minimized.

@kenotron

kenotron Jul 16, 2018

Collaborator

Not sure under what circumstance you wouldn't be able to get the file size. I'm thinking maybe we should be faililng the build if we can't even determine the file size here. This way, we wouldn't have a "loophole" for slipping through size issues. We probably should just throw e instead of return -1.

@kenotron

kenotron Jul 16, 2018

Collaborator

Not sure under what circumstance you wouldn't be able to get the file size. I'm thinking maybe we should be faililng the build if we can't even determine the file size here. This way, we wouldn't have a "loophole" for slipping through size issues. We probably should just throw e instead of return -1.

This comment has been minimized.

@dzearing

dzearing Jul 18, 2018

Collaborator

maybe just remove the try/catch here. No need to hide and obfuscate the actual exception.

@dzearing

dzearing Jul 18, 2018

Collaborator

maybe just remove the try/catch here. No need to hide and obfuscate the actual exception.

Claudiazhaoya added some commits Jul 16, 2018

@cliffkoh

This comment has been minimized.

Show comment
Hide comment
@cliffkoh

cliffkoh Jul 17, 2018

Collaborator

Appveyor isn't running

image

We would need Appveyor to be running to validate this change (and for this check in to be meaningful! :))

Collaborator

cliffkoh commented Jul 17, 2018

Appveyor isn't running

image

We would need Appveyor to be running to validate this change (and for this check in to be meaningful! :))

@iclanton iclanton closed this Jul 17, 2018

@iclanton iclanton reopened this Jul 17, 2018

@dzearing dzearing merged commit c5b4857 into master Jul 19, 2018

7 checks passed

bundlesize ../apps/test-bundle-button/dist/test-bundle-button.min.js: 39.53KB < maxSize 51.5KB gzip (same as master)
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
screener Screener visual tests accepted
Details

@micahgodbolt micahgodbolt deleted the SizeAuditorTest branch Jul 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment