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.

Sign up for GitHub

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

Jump to bottom

Fix/file size reporter #5025

Merged
merged 2 commits into from
Sep 19, 2018
Merged

Fix/file size reporter #5025

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea92b9c
fix FileSizeReporter in Windows enviroment
Sep 18, 2018
fe97232
chore: change line endings
fiddep Sep 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/react-dev-utils/FileSizeReporter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ function printFileSizesAfterBuild(
name: path.basename(asset.name),
size: size,
sizeLabel:
filesize(size) + (difference ? ' (' + difference + ')' : '')
filesize(size) + (difference ? ' (' + difference + ')' : ''),
};
})
)
Expand Down Expand Up @@ -98,6 +98,7 @@ function printFileSizesAfterBuild(
function removeFileNameHash(buildFolder, fileName) {
return fileName
.replace(buildFolder, '')
.replace(/\\/g, '/')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to just support both path separators in the below regex, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow.
The path are resolved differently from the start. These screens are before the change.
measureFileSizesBeforeBuild -> http://prntscr.com/kvxlyq
printFileSizesAfterBuild -> https://prnt.sc/kvxm6u

My solution was to just make sure they were always the same, if there is a smarter way sadly I'm not seeing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I should've been clearer.
Is this because asset.name always returns paths with forward slashes (/)? If so, this patch makes perfect sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. While measureFileSizesBeforeBuild returns the path delimiter for the current os.

.replace(
/\/?(.*)(\.[0-9a-f]+)(\.chunk)?(\.js|\.css)/,
(match, p1, p2, p3, p4) => p1 + p4
Expand Down