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

Notify when less.js is done processing #2226

Merged
merged 2 commits into from
Oct 17, 2014

Conversation

levithomason
Copy link
Contributor

Fixes issue #283 "How to tell when less is done processing ?"

Problem
When using less.js in the browser it is sometimes necessary to wait for less.js to finish processing and applying new css values before running additional code or tests. However, there is no way to tell when less.js is done. This means developers are using timeouts, intervals, and other hacky code as a work around.

Use cases

  • Executing code that is depending on less.js having finished processing.
  • Delaying E2E tests for an in browser theme customizer until less.js has completed its refresh().
  • Avoiding FOUC by hiding elements and displaying them after less.js has completed.

Solutions (thus far proposed)

  • Fire an event after procesing.
  • Call a callback function after LESS is done processing.
  • Have less.refresh() return a promise.

pass the error to reject()
@levithomason
Copy link
Contributor Author

@lukeapage this PR is in flux until a solution is agreed on and implemented.

After reviewing the code, use cases, issues, and proposed solutions it seems we can solve all use cases by having refresh() return a promise and firing an event at the time it is resolved. This way those calling refresh() manually will have a then() method and those just loading less.js in the browser will get an event they can listen for. I'm imagining usage something like:

Event

less.on('processed', function() {
    // do stuff
});

or

Promise

var reload = true;
var modifyVars = { '@foo': 'bar' };

less.refresh(reload, modifyVars).then(
    function() {
        // do stuff with assurance css has be process and @foo is now 'bar'
    },
    function(e) {
        // e contains the error object return by loadStyleSheet callback
    }
);

The current changes, at a0b1b2f, allow for the promise use cases without breaking current tests.

As for the event use case, it seems integrating something like EventEmitter or emitter and mixing it in with the less.js prototype would work well.

Regarding:

...we can make refresh return a promise and just set less.xxx to be that promise for the first scan...
...exposing an extra field (the thing :)) on the less object is necessary...

I'd like to hear more as I don't think I understand the purpose of having less.xxx as a promise nor why a less.xxx is necessary. Thanks!

@lukeapage
Copy link
Member

Cool, looks good, so now on the last line of index where it calls refresh for the first time you could assign it to less.something where something is a sensible name . Then people have a way of detecting the initial parse being done.

fix white space

return promise on modifyVars
@levithomason
Copy link
Contributor Author

Alright, the current code does not include any event firing but does return a promise for refresh() and modifyVars(). Also, the promise is assigned to less.hasFinished so it's then-able without having to modify vars or refresh.

Uses

less.refresh(reload, modifyVars).then(...);
less.modifyVars(record).then(...);
less.hasFinished.then(...);

Is this what you had in mind?

The resolve receives some info about the processing we already had available (start time, end time, total ms, and # of sheets).

@lukeapage
Copy link
Member

Yes, will take a look soon.

@lukeapage
Copy link
Member

thanks!

lukeapage added a commit that referenced this pull request Oct 17, 2014
@lukeapage lukeapage merged commit 3166564 into less:2_0_0 Oct 17, 2014
@levithomason levithomason deleted the feature/notify-on-finish branch October 22, 2014 03:53
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