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

Upgrade minifier #891

Merged
merged 1 commit into from
Oct 16, 2014
Merged

Upgrade minifier #891

merged 1 commit into from
Oct 16, 2014

Conversation

matthiasmullie
Copy link
Member

No description provided.

@WouterSioen
Copy link
Member

This one worked, thanks :)

@jessedobbelaere
Copy link
Member

I deployed a website today but changed the minifier to version 1.3.* and installed the vendors. The website was working perfectly with debug enabled, but when I disabled debug all pages worked except the page with my "events" module. I got a blank page and it kept loading and loading, so maybe a cache problem. I installed v1.1 of the minifier and the website worked instantly. (Fork CMS version 3.6.6). Maybe there are still some issues with this version of the minifier? Or isn't it compatible with fork 3.6.6? I'll try to deploy the website to a subdomain and recreate & show the problem...

edit: website with v1.3.2 minifier and website with v1.1 minifier. Visit the "calendar" page. It's a very simple module with a single div in the template, it loads the fullcalendar.js plugin, fetches the calendar items with an ajax call, and displays them in a calendar. Is there some way that I can detect where exactly it goes wrong during the minification proces?

@WouterSioen
Copy link
Member

Is it an open source module? It could be useful to test this with the exact
same asset files.

Op vrijdag 10 oktober 2014 heeft Jesse Dobbelaere notifications@github.com
het volgende geschreven:

I deployed a website today but changed the minifier to version 1.3.* and
installed the vendors. The website was working perfectly with debug
enabled, but when I disabled debug all pages worked except the page with my
"events" module. I got a blank page and it kept loading and loading. I
installed v1.1 of the minifier and the website worked instantly. (Fork CMS
version 3.6.6). Maybe there are still some issues? I'll try to deploy the
website to a subdomain and recreate the problem...


Reply to this email directly or view it on GitHub
#891 (comment).

@matthiasmullie
Copy link
Member Author

I assume fullcalendar.js (http://fullcalendar.io/download/?) is a rather elaborate JS project.

Minifying takes some time, which is why I think you didn't see anything: minifying a really big JS file takes a while.
1.1 probably did a way faster job because it is much less strict (e.g. it also strips some things where shouldn't, like in strings or regular expressions) & thorough (it doesn't strip as much as latest versions)

I've just updated minifier to 1.3.3, which will cut down minify time up to about half. I'll update this pull request accordingly.

However, if you include external scripts that already have a minified version, you should make sure Fork CMS doesn't run it through the minifier again. This is done by setting the 2nd parameter of Header::addJS to false, like:

$this->addJS('your-minified-file.js', false);

Also: while minification may slow down a bit, it will only happen once. After that, all visitors will get the already minified file ;)

-- I'll update this PR to 1.3.3, which has some performance improvements

@matthiasmullie
Copy link
Member Author

I've updated the PR - should now be 1.3.3

@jessedobbelaere
Copy link
Member

Alright, yes the fullcalendar plugin is a lot of code to minimize. I'll start adding the false parameter. However, the 1.1 version doesn't seem to have a problem to minimize & show the page?

@matthiasmullie
Copy link
Member Author

1.1 could cause problems. Usually, it will be fine though.

1.1. didn't properly check where it was stripping whitespace or comments, so it could remove whitespace or things that look like comments inside strings or regular expressions (in which case it shouldn't be removed). If the script had no such things, 1.1 would indeed work just fine. It's just not really safe to use :p

@WouterSioen WouterSioen merged commit 5d93044 into forkcms:master Oct 16, 2014
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.

None yet

3 participants