register_extension handlers can't do async IO (eg, `less` @imports fail) #203

Closed
k1w1 opened this Issue Jul 1, 2012 · 15 comments

Projects

None yet

8 participants

@k1w1
k1w1 commented Jul 1, 2012

lf .less files with @import are used then the register_extension function returns before the less.render() callback. This can mean that the updated application is served before the .css resource is added to the bundle.

The comment in the source code indicates that the author thought the callback was synchronous, but it is actually asynchronous.

This is probably the cause of this issue too:

http://stackoverflow.com/questions/10442201/meteor-how-to-use-multiple-less-files?lq=1

@k1w1
k1w1 commented Jul 1, 2012

Rather than the size of the .less file, it might be the use of @import that is making the less parser asynchronous. Here is the .less file that causes the failure:

@import "public/bootstrap/less/bootstrap.less";

table.toolbar {
td {
border: 1px solid #444; @grayLight;
}
}

@debergalis
Member

Okay, sure looks like you're right. Either the callback to register_extension should take a callback itself, or we need to Fiberize the whole thing. Marking confirmed, thank you.

@glasser
Member
glasser commented Oct 10, 2012

By reading the code, I agree that the problem is @import and not just large files.

@gschmidt
Member
gschmidt commented Dec 4, 2012

@k1w1, thanks, good catch! The root problem here is that the register_extension callbacks are not run in a fiber and don't have a way to block for IO. Core team will take this on as part of our current project to overhaul the package system.

@jperl
Contributor
jperl commented Jan 24, 2013

@gschmidt Awesome, looking forward to this

@jperl jperl referenced this issue in FoundOPS/BillsTools Jan 24, 2013
Open

Split up less #1

@n1mmy
Member
n1mmy commented Jan 25, 2013

This is fixed on the engine branch and will be merged to devel soon.

@avital avital added a commit that referenced this issue Jan 26, 2013
@avital avital Fix #203: support less @import directives 0c814a2
@avital avital added a commit that referenced this issue Jan 26, 2013
@avital avital Fix #203: support less @import directives 6745745
@avital
Contributor
avital commented Jan 26, 2013

Should be fixed on devel.

@avital avital closed this Jan 26, 2013
@mrcoles
mrcoles commented Jan 26, 2013

I just checked out the devel branch and the @import directives are working indeed. However, it looks like meteor is less forgiving about errors in Less compilation now. Take the following error-ridden code:

.test(@arguments) {}                                                            
.p { .test(1px, 2px); }

With the code on the master branch of meteor, the Less error is caught and meteor gives the "Your application is crashing. Waiting for file change." message.

With the code on the devel branch, meteor crashes and I need to re-run meteor once I've fixed the bug. Is this unintended?

@avital
Contributor
avital commented Jan 28, 2013

Sounds like a bug. I'll take a look.
On Jan 26, 2013 6:08 AM, "Peter Coles" notifications@github.com wrote:

I just checked out the devel branch and the @import directives are
working indeed. However, it looks like meteor is less forgiving about
errors in Less compilation now. Take the following error-ridden code:

.test(@arguments) {}
.p { .test(1px, 2px); }

With the code on the master branch of meteor, the Less error is caught and
meteor gives the "Your application is crashing. Waiting for file change."
message.

With the code on the devel branch, meteor crashes and I need to re-run
meteor once I've fixed the bug. Is this unintended?


Reply to this email directly or view it on GitHubhttps://github.com/meteor/meteor/issues/203#issuecomment-12735761.

@avital
Contributor
avital commented Jan 28, 2013

I put the code you posted in a .less file in a meteor app and added the less package. Both on master and on devel I get the following. Pointing my browser to http://localhost:3000 shows me the error as well.

[[[[[ ~/skel2 ]]]]]

Running on: http://localhost:3000/
Errors prevented startup:
/Users/avital/skel2/foo.less: Less compiler error: Object #<Object> has no method 'toCSS'
/Users/avital/skel2/foo.less: Less compiler error: Object #<Object> has no method 'toCSS'
Your application is crashing. Waiting for file change.
@mrcoles
mrcoles commented Jan 29, 2013

If you put that code in a less file that’s importing another less file, then the error happens. Without an import, it catches it fine and waits for a fix.

Here's a gist of the example code https://gist.github.com/4660618 less-test.less imports imports.less and then it raises a compilation error and crashes meteor.

@avital
Contributor
avital commented Jan 29, 2013

Thanks for the example. I'll take another look.
On Jan 28, 2013 4:41 PM, "Peter Coles" notifications@github.com wrote:

If you put that code in a less file that’s importing another less file,
then the error happens. Without an import, it catches it fine and waits for
a fix.

Here's a gist of the example code https://gist.github.com/4660618
less-test.less imports imports.less and then it raises a compilation
error and crashes meteor.


Reply to this email directly or view it on GitHubhttps://github.com/meteor/meteor/issues/203#issuecomment-12814250.

@avital avital reopened this Jan 30, 2013
@glasser
Member
glasser commented Jan 31, 2013

I also managed to get an error of the form:

$ run-meteor packages/less/
[[[[[ ~/Projects/Meteor/meteor/packages/less ]]]]]

Running on: http://localhost:3000/

/Users/glasser/Projects/Meteor/meteor/dev_bundle/lib/node_modules/less/lib/less/parser.js:413
                        throw new(LessError)(e, env);
                              ^
[object Object]
$

(when i replaced less_test_constants.less with the empty file, ie, I made a @foo fail)

which (a) is a terrible error and (b) broke the re-run loop.

@glasser
Member
glasser commented Feb 6, 2013

OK, I looked into this a little more. There is some definite weirdness in less.render, where it can call a toCSS which can throw, even though it's in a context where it's supposed to return errors via callback.

However, upgrading from less 1.3.1 to the newest version (1.3.3) has several improvements:

  • The "toCss" error is improved: less/less.js#990
  • We can pass a syncImport option to less.render which means that less will work fully synchronously, which makes our code much simpler.
  • You can now do @import "foo.lessimport" (or any extension made up of letters) and less will not try to add .less to the end. This way, Meteor won't process your import files on their own.

So upgrading less should fully fix this issue.

@glasser
Member
glasser commented Feb 6, 2013

This should be fixed by 453679d on devel.

@glasser glasser closed this Feb 6, 2013
@mitar mitar pushed a commit to mitar/blaze-pruned that referenced this issue Dec 5, 2016
@glasser glasser Upgrade less to 1.3.3.
Should improve the error message in meteor/meteor#203
(see less/less.js#990) though it may not fix the
way that the error is handled.
b5d186a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment