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.

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

Added minification of build files. Resolves #4. #6

Closed
wants to merge 1 commit into from

Conversation

fabianmarz
Copy link
Member

Added an additional minified version of the build file using clean-css. Currently using the default settings, but we could also consider using level 2 optimizations. This should be handled with caution and needs testing first.

@sun sun requested a review from colourgarden August 30, 2017 12:19
@sun
Copy link
Member

sun commented Aug 30, 2017

Shouldn't we see some/any kind of "quality/performance improvement" in the reported tests with a change like this?

@LucaPipolo
Copy link
Contributor

LucaPipolo commented Aug 30, 2017

@sun — as you can see below test run. Not all of them are configured to check Gulpfile and not all of them in the same way. I thought was not necessary at the moment.

But, e.g., Codacy with is enabled to check with ESLint also the Gulpfile, is reporting:
https://www.codacy.com/app/LucaPipolo/bacon/pullRequest?prid=874866

@sun
Copy link
Member

sun commented Aug 31, 2017

@LucaPipolo Codacy apparently reports a negative effect (due to false-positive coding style issues).
That is kind of misleading information, as it is the opposite (negative) direction of the originally intended (positive) change of this PR.

My question was rather pointing into the direction of whether we're able to track/profile the quality / size / performance / stability / validity / compatibility of the compiled frontend assets with any of these tools — especially because the description of this particular PR here states:

This should be handled with caution and needs testing first.

(emphasis mine)

Aside from that, is Codacy the only tool that checks the code in gulpfile.js? Or is it just the only tool for which the coding style settings for JavaScript code haven't been configured yet?

@LucaPipolo
Copy link
Contributor

@sun — yep, Codacy is reporting a not passed status because it's analyzing also the Gulpfile, without using the ESLint file.

E.g. Code Climate, instead, is analyzing Gulpfile as well but marked it as passed/A (see here).

About:

My question was rather pointing into the direction of whether we're able to track/profile the quality / size / performance / stability / validity / compatibility of the compiled frontend assets with any of these tools — especially because the description of this particular PR here states:

Sorry but can not follow you. Yes, of course we're able to check these things of the compiled frontend assets but we're not doing it. dist/ folder is not pushed so the quality inside is not checked.

And, my question is, why should we? We're checking quality of all sources files (.css, .scss, .js, .coffee, etc.) why also checking for compiled frontend assets? In addition, was not the idea to never push the dist/ folder since it should contain minified and compressed output?

However, we can do that. :)

@fabianmarz
Copy link
Member Author

fabianmarz commented Aug 31, 2017

The service should run the gulpfile to create its own dist directory to test against. I see no need to push the build files. Is this possible?

Sorry for misleading…

This should be handled with caution and needs testing first.

Was meant to be related to the level 2 optimizations of clean-css, not the PR in total.

@LucaPipolo
Copy link
Contributor

@fabianmarz

The service should run the gulpfile to create its own dist directory to test against. I see no need to push the build files. Is this possible?

It's seems more something done by services like Circle CI that, running test, can look the package.json, download the NPM packages and run things like gulp, bower and so on.

So I'd say these are not the right tools to compile the dist folder and then check the output without commit it.

@sun
Copy link
Member

sun commented Aug 31, 2017

Sure, we certainly don't want to commit the compiled files in dist. But we should be able to track the quality of the compiled code, too.

That would be the point at which we would possibly see a positive improvement for PRs like this.

At least Scrutinizer can be configured to perform a full build with composer, npm, and gulp, so as to be able to run automated tests (including frontend tests) but also perform quality checks on the compiled results.

@colourgarden colourgarden mentioned this pull request Sep 1, 2017
@colourgarden
Copy link
Contributor

Closing this off as it's already been merged in #10. clean-css and rename have been included in the Bacon gulpfile for usage with the production-css task. Ref

@LucaPipolo LucaPipolo deleted the minification-build-fab branch November 2, 2018 17:00
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.

4 participants