cgross / generator-cg-angular

Yeoman generator for Enterprise Angular projects.
MIT License
592 stars 198 forks source link

Changing app.less not reloading #60

Closed douglascorrea closed 10 years ago

douglascorrea commented 10 years ago

Hi!

The generator-cg-angular is amazing, thank you for the good work.

This is more like a question than an issue: I'm updating app.less, or any other .less file, and the styles is not being applied on the browser. I mean, I see the less files being reloaded in console but the style are not really applied.

Am I missing something?

cgross commented 10 years ago

Livereload doesn't really work great with LESS. It tries to be smart and reload the LESS styles w/o reloading the whole page. But this doesn't work so well. It only adds the styles all over again but does not remove your old styles. If you look at an element in the dev tools, you'll see multiple rules for the same rule in your LESS file (one rule for each time it was reloaded). Then the old rules and your new rules are all combined together. This sucks because if you had some values set that you removed, the old rules are going to continue to apply those values.

tl;dr - Sometimes LESS reload works, sometimes it doesn't. This is a problem with the LESS client js and isn't specific to this generator (or even grunt or grunt watch).

bpartridge commented 10 years ago

+1 to this bug; I do believe that since this (wonderful) generator promises to integrate LiveReload and client-side LESS out of the box, it should provide a workaround, or at least mention how to make a manual workaround in the README.

After some digging, it turns out that LESS does have a relatively robust facility for removing old style tags for files that it's handling (since client-side libraries may add link tags at any time), but since LiveReload attempts to bust caching by adding a ?livereload=TIMESTAMP query parameter to each link tag it adds to the head, LESS can't figure out that the new link is actually meant to refer to the same .less file... so old style tags accumulate. It's not LESS's fault - you can imagine a web app that loads various different LESS files that are distinguished only by their query strings, but which are meant to coexist.

The easiest solution I found was to:

function extractId(href) {
    return href.replace(/^[a-z-]+:\/+?[^\/]+/, '' )  // Remove protocol & domain
        .replace(/[\?\&]livereload=\w+/,   '' )  // Remove LiveReload cachebuster
        .replace(/^\//,                 '' )  // Remove root /
        .replace(/\.[a-zA-Z]+$/,        '' )  // Remove simple extension
        .replace(/[^\.\w-]+/g,          '-')  // Replace illegal characters
        .replace(/\./g,                 ':'); // Replace dots with colons(for valid id)
}

And you get reloading LESS!

A longer-term solution would be to fork LESS and refer to the fork in this project, or to submit a pull request to LESS allowing the extractId method to be overridden, then override it in app.js.

@cgross - let me know if you think I should proceed with any of these approaches. This is a great generator and I'd love for it to have LESS reloading working!

cgross commented 10 years ago

@bpartridge Wow, great investigation! Makes alot more sense after you've put all the pieces together. I'm surprised other projects haven't had this problem. It feels like we're the first people to deep dive into this problem.

The problem is a edge case of the two libraries not working together perfectly. One could argue that the livereload.js shouldn't append that query param for cache busting. One could also argue that is totally appropriate and the less.js should account for it. Coming at it pragmatically, the livereload.js (the livereload browser javascript code) is shared by quite a few projects as I understand it. So if the LESS folks would make a change to extractId like you mentioned, that'd be the easiest way to solve this problem.

So yea, I'm totally in favor of a pull request to the LESS.js repo. It looks like the relevant code in the LESS.js repo is here (coffeescript): https://github.com/less/less.js/blob/93d6b0876eb36a530cf74f37e6aa663b5c5d8b0a/lib/less/browser.js#L67

Thanks for digging into this. I had spent an hour or two looking into it a while back and, while I did get some understanding of the issue, I didn't get to the root of it like you did. If you can get the PR accepted you'll make alot of people happy. I know my day-to-day development will be alot nicer not having to futz with refreshing sometimes (wait, did i update a less file or just a js/html file...?) and not others.

adnasa commented 10 years ago

This is great work, @bpartridge I have to admit though that I did resolve my problem by registering a grunt task of building app.less to app.css and change the reference in the main file.

cgross commented 10 years ago

FYI - the latest release of less.js includes the fix for this. If you upgrade to less 2.1 in your projects, you should see the reload of less styles working!

douglascorrea commented 10 years ago

@cgross thx for the update. Actually I updated it to use stylud and build app.css automatically with livereload.