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

Cleancss fix #509

Merged
merged 3 commits into from
Jun 5, 2017
Merged

Cleancss fix #509

merged 3 commits into from
Jun 5, 2017

Conversation

matteofigus
Copy link
Member

@matteofigus matteofigus commented Jun 5, 2017

With the recent upgrade of oc-template-jade (#497) using a patched jade version with security fixes, the uglify-css filter has gone in favour of newest clean-css. This means that components using this filter in production would break.

This introduces support for the old filter, allowing a clean migration to the new filter (by publishing a new component that uses clean-css instead of uglify-css.

In order to test it works, I added a component that has all the filters (jade-filters) which I test doesn't generate any compilation error during rendering.

Copy link
Member

@mattiaerre mattiaerre left a comment

Choose a reason for hiding this comment

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

@matteofigus I am not sure where this jstransformer-uglify-css is applied as I can see only test related files; is this basically another jade compiler?

@@ -0,0 +1,3 @@
_package
package.tar.gz
node_modules
Copy link
Member

Choose a reason for hiding this comment

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

newline

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,5 @@
var sum = function(x, y){
Copy link
Member

Choose a reason for hiding this comment

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

const

Copy link
Member Author

@matteofigus matteofigus Jun 5, 2017

Choose a reason for hiding this comment

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

This is client-side js that runs through uglify, can't be ES6

Copy link
Member

Choose a reason for hiding this comment

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

kk

}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

newline

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@matteofigus
Copy link
Member Author

@mattiaerre any time jade/pug encounters a filter, it tries to resolve the filter by requiring jstransformer-${filterName}. By renaming the uglify-css to clean-css they basically broke retro-compatibility. As you can see from the package.json, I created a filter that adds the uglify-css alias back: https://github.com/matteofigus/jstransformer-uglify-css/blob/master/index.js

@matteofigus
Copy link
Member Author

This is the code that the jade-legacy removed in favour of migrating to jstransformers, which basically broke the uglify-css filter: jnordberg/jade-legacy@0ec5fc6

@mattiaerre
Copy link
Member

@matteofigus this LGTM 👍

@mattiaerre mattiaerre merged commit ff3ab07 into master Jun 5, 2017
@matteofigus
Copy link
Member Author

Thanks for your quick help @mattiaerre

@mattiaerre mattiaerre deleted the cleancss-fix branch June 5, 2017 17:41
@mattiaerre
Copy link
Member

mattiaerre commented Jun 5, 2017

@matteofigus let me know if there is anything else I can do in order to help you w/ testing this new functionality once it will be released

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.

2 participants