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

Fix synchronously loading/applying stylesheets on page load. #2575

Merged
merged 1 commit into from
Apr 29, 2015

Conversation

chipx86
Copy link
Contributor

@chipx86 chipx86 commented Apr 28, 2015

Starting in 2.0, stylesheet loading became asynchronous, through the
usage of promises for both calculating the list of stylesheets and the
initial call to less.refresh(). This resulted in visual issues while
loading on some browsers (noticed in Firefox and Safari), along with
breakages of any custom JavaScript that depended on the computed style
of elements on the page, due to race conditions.

This change preserves the promise for initial page loading, in order to
retain support for less.pageLoadFinished, but immediately executes the
stylesheet scan (through a new less.registerStylesheetsImmediately
function) and the less.refresh() call. That resulting behavior matches
versions of less prior to 2.0.

This unveiled a regression in registering functions, both in the browser
and in unit tests, that was not previously noticed due to the
asynchronous load. Registered functions would have a less variable set
to the less options, and not less itself, when not going through the
asynchronous loading mode. This meant that both unit tests and
real-world function registration would break when the sync page loading
was fixed. Overriding window.less to point to the actual less module and
not less.options during bootstrap fixes this.

This fixes #2317.

Starting in 2.0, stylesheet loading became asynchronous, through the
usage of promises for both calculating the list of stylesheets and the
initial call to less.refresh(). This resulted in visual issues while
loading on some browsers (noticed in Firefox and Safari), along with
breakages of any custom JavaScript that depended on the computed style
of elements on the page, due to race conditions.

This change preserves the promise for initial page loading, in order to
retain support for less.pageLoadFinished, but immediately executes the
stylesheet scan (through a new less.registerStylesheetsImmediately
function) and the less.refresh() call. That resulting behavior matches
versions of less prior to 2.0.

This unveiled a regression in registering functions, both in the browser
and in unit tests, that was not previously noticed due to the
asynchronous load. Registered functions would have a 'less' variable set
to the less options, and not less itself, when not going through the
asynchronous loading mode. This meant that both unit tests and
real-world function registration would break when the sync page loading
was fixed. Overriding window.less to point to the actual less module and
not less.options during bootstrap fixes this.

This fixes less#2317.
(links[i].type.match(typePattern)))) {
less.sheets.push(links[i]);
}
less.registerStylesheetsImmediately = function() {
Copy link
Member

Choose a reason for hiding this comment

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

you don't call less.registerStylesheetsImmediately from anywhere? and you have removed the call to less.registerStylesheets and that isn't called anywhere..

I guess you just need to call less.registerStylesheetsImmediately from bootstrap ?

otherwise, many thanks for tracking it down and fixing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the GitHub bug I mentioned in issue #2317. The line is actually there, right above the less.refresh call and promise assignment in bootstrap.js. However, GitHub appears to have a bug in the diff viewer where a delete line at the end of a file + adding a missing trailing newline to the file + adding a line results in that line getting skipped. I've already let GitHub know about it.

This is what the diff looks like for the change:

--- a/lib/less-browser/bootstrap.js
+++ b/lib/less-browser/bootstrap.js
@@ -13,14 +13,13 @@ require("./add-default-options")(window, options);

 var less = module.exports = require("./index")(window, options);

+window.less = less;
+
 if (options.onReady) {
     if (/!watch/.test(window.location.hash)) {
         less.watch();
     }

-    less.pageLoadFinished = less.registerStylesheets().then(
-        function () {
-            return less.refresh(less.env === 'development');
-        }
-    );
-}
\ No newline at end of file
+    less.registerStylesheetsImmediately();
+    less.pageLoadFinished = less.refresh(less.env === 'development');
+}

If you clone my tree and git show the commit, you'll see the line is there.

Also, would you mind trying to restart the Travis-CI build? They had some problems last night with random stalls, which was randomly affecting different builds at different points during the build process, triggering failures of 3 different types that I had noticed. Hoping that was some temporary issue on their end related to their failed upgrade and rollbacks.

@lukeapage
Copy link
Member

Sure, restarted...

@lukeapage
Copy link
Member

Thanks for this.

lukeapage added a commit that referenced this pull request Apr 29, 2015
Fix synchronously loading/applying stylesheets on page load.
@lukeapage lukeapage merged commit fbbd313 into less:master Apr 29, 2015
@lukeapage
Copy link
Member

I'll release soon

@chipx86 chipx86 deleted the fix-sync-loading branch April 29, 2015 07:54
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.

Latest less.js (2.1.1) behaves async in IE
2 participants