Skip to content

Conversation

@k0nserv
Copy link

@k0nserv k0nserv commented Aug 8, 2017

For prod and SSR builds include optimize-css-assets-webpack-plugin to
optimize duplication in CSS output.

Copy link

@matthewdavidson matthewdavidson left a comment

Choose a reason for hiding this comment

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

Needs a changelog entry too.

LGTM otherwise

Choose a reason for hiding this comment

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

Whats the thinking here? i.e .why would we need to expose those options to userland config? We don't do this for autoprefixer for instance...

Choose a reason for hiding this comment

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

Also if there is a good reason to do that, then it would need documentation.

Copy link
Author

Choose a reason for hiding this comment

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

I figured we'd ship it silently to futureproof when someone inevitably finds something they want to tweak which is why I didn't include docs

Choose a reason for hiding this comment

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

I see. I guess the whole premise of create-react-app (and thus backpack-react-scripts) is to hide the configuration away from consumers. If there is something that a consumer / team deems necessary to include then why wouldn't we wanna include that for everyone? Im tempted to remove it.

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough. I’ll remove it 👍

@k0nserv k0nserv force-pushed the BPK-838-add-optimize-css-assets-plugin branch from d736132 to 1ac221a Compare August 8, 2017 15:58
Copy link
Author

Choose a reason for hiding this comment

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

Matt thoughts on this. My thinking is that this is a significant change in output result so it should be breaking even though no change is required by the user. Erring on the side of caution

Choose a reason for hiding this comment

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

Is it going to break builds? Is it going to break visual layout of apps?

If the answer is no to both of them then I think it's a fix.

Copy link
Author

Choose a reason for hiding this comment

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

Nope it shouldn't affect anything, but it's a more aggressive optimizer so it could introduce issues.

Choose a reason for hiding this comment

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

We can do a smoke test on some internal projects to get confidence?

@k0nserv k0nserv force-pushed the BPK-838-add-optimize-css-assets-plugin branch 2 times, most recently from e1bf4ef to 9746fdb Compare August 9, 2017 12:10
@k0nserv
Copy link
Author

k0nserv commented Aug 9, 2017

@matthewdavidson Have a look again please

@matthewdavidson matthewdavidson self-assigned this Aug 10, 2017
Copy link

@matthewdavidson matthewdavidson left a comment

Choose a reason for hiding this comment

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

I've verified this on an internal project. :shipit:

Choose a reason for hiding this comment

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

unnecessary new line

For prod and SSR builds include `optimize-css-assets-webpack-plugin` to
optimize duplication in CSS output.
@k0nserv k0nserv force-pushed the BPK-838-add-optimize-css-assets-plugin branch from 9746fdb to 1180252 Compare August 10, 2017 10:26
@k0nserv k0nserv merged this pull request into master Aug 10, 2017
@k0nserv k0nserv deleted the BPK-838-add-optimize-css-assets-plugin branch August 10, 2017 10:42
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.

3 participants