Skip to content

Conversation

@rkostrzewski
Copy link
Collaborator

Closes #103

lukeed
lukeed previously approved these changes Jun 26, 2017
Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Lol, easy enough! Embarrassed I didn't notice this was missing

@rkostrzewski
Copy link
Collaborator Author

That was my third try!

developit
developit previously approved these changes Jun 26, 2017
Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

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

Who would have thought

@developit
Copy link
Member

Build seems to be failing due to the tests now seeing .map files (yay!!)

@reznord
Copy link
Member

reznord commented Jun 26, 2017

Build seems to be failing due to the tests now seeing .map files (yay!!)

@developit We need to fix the tests too, so it looks for the main styles.css instead of .map file right?


if (keys.length === 1 && keys[0] === 'size' && typeof keys[0] === 'number') {
return { size: Math.round(obj.size / 10) * 10 };
if (keys.length === 1 && keys[0] === 'size' && typeof obj.size === 'number') {
Copy link
Collaborator Author

@rkostrzewski rkostrzewski Jun 26, 2017

Choose a reason for hiding this comment

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

Turns out typeof key === 'number' is never true... wonder why...

Anyway sizes are now compared correctly (rounded to nearest 50 bytes for comparision) and without it would never know that push-manifest.json included source maps. 🙈

@rkostrzewski rkostrzewski dismissed stale reviews from lukeed and developit June 26, 2017 19:48

More significant changes added

@rkostrzewski
Copy link
Collaborator Author

Seems like snapshots have different sizes on CI because of absolute paths inside maps. Gonna investigate.

@lukeed
Copy link
Member

lukeed commented Jun 27, 2017

I saw that. Would it make sense to just check file < size? Dependency upgrades may slightly after the file outputs in the future. Would be a pity to have failing tests because filesize changed ±1 byte.

@rkostrzewski
Copy link
Collaborator Author

I was thinking about checking it like this: expected - delta =< actual <= expected + delta. Coz too small could mean no output produced

@lukeed
Copy link
Member

lukeed commented Jun 27, 2017

Yeah, or within 5% of expected. Either way would work imo. 👍

Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

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

new diff sizing looks good.


for (let filename in compilation.assets) {
if (/route-/.test(filename)) {
if (/route-/.test(filename) && !/\.map$/.test(filename)) {
Copy link
Member

Choose a reason for hiding this comment

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

ooh good catch.

@developit
Copy link
Member

I wonder if there's a way to get webpack to avoid the absolute paths there? Seems like they could be normalized to be relative to context. (totally not in this PR)

@rkostrzewski
Copy link
Collaborator Author

@developit seems like module aliases & asnyc-component-loader don't play nicely.

@rkostrzewski rkostrzewski merged commit e6ca781 into master Jun 27, 2017
@rkostrzewski rkostrzewski deleted the feature/prod-source-maps branch June 27, 2017 18:18
@rkostrzewski rkostrzewski added this to the 1.2.0 milestone Jun 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants