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

show meaningful less errors #244

Merged
merged 9 commits into from Mar 21, 2013

Conversation

4 participants
@vicapow
Contributor

vicapow commented Mar 18, 2013

show this...

error: Less compilation error
FileError: '../flatstrap/less/bootstrap.less' wasn't found.

1 @import "../flatstrap/less/bootstrap.less";
2 @import "../flatstrap/less/responsive.less";

instead of this...

verbose: Server stopped.

/example/node_modules/sails/node_modules/less/lib/less/parser.js:390
                        else                                  throw new(LessEr
                                                                    ^
[object Object]
@vicapow

This comment has been minimized.

Show comment
Hide comment
@vicapow

vicapow Mar 18, 2013

Contributor

still issues. closing for now.

Contributor

vicapow commented Mar 18, 2013

still issues. closing for now.

@vicapow vicapow closed this Mar 18, 2013

@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Mar 18, 2013

Member

@vicapow excited to see your solution!

Member

mikermcneil commented Mar 18, 2013

@vicapow excited to see your solution!

vicapow added some commits Mar 19, 2013

properly set the less path so less @import statements to files outsid…
…e of the

asset directory work properly. more properly log less error messages

@vicapow vicapow reopened this Mar 19, 2013

@vicapow

This comment has been minimized.

Show comment
Hide comment
@vicapow

vicapow Mar 19, 2013

Contributor

there we go! now it should at least report the error. still not able to get the line number info since all the less files in the assets/styles folder are clobbered into a single string that is passed to less.

this commit also fixes issues with using the @import statement in less to files outside of the assets folders

Contributor

vicapow commented Mar 19, 2013

there we go! now it should at least report the error. still not able to get the line number info since all the less files in the assets/styles folder are clobbered into a single string that is passed to less.

this commit also fixes issues with using the @import statement in less to files outside of the assets folders

@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Mar 19, 2013

Member

@vicapow Thanks!

I think the reason the files are concatenated is to allow mixins/variables to work-- has anyone seen a way to get original line numbers/files in this sort of a setup? @techpines any ideas?

Member

mikermcneil commented Mar 19, 2013

@vicapow Thanks!

I think the reason the files are concatenated is to allow mixins/variables to work-- has anyone seen a way to get original line numbers/files in this sort of a setup? @techpines any ideas?

@techpines

This comment has been minimized.

Show comment
Hide comment
@techpines

techpines Mar 19, 2013

Member

@vicapow I would love to be able to fix this upstream in asset-rack. Is this what's needed to make the less errors come out correctly?

if (error) {
   if(!(error instanceof Error)){
        less.writeError(error)
        error = new Error("Less asset compilation error");
    }
    return self.emit('error', error);
}

Technically, the way it is currently set up, I think it is outputting the error "[object Object]" just not in a nonusable form.

Member

techpines commented Mar 19, 2013

@vicapow I would love to be able to fix this upstream in asset-rack. Is this what's needed to make the less errors come out correctly?

if (error) {
   if(!(error instanceof Error)){
        less.writeError(error)
        error = new Error("Less asset compilation error");
    }
    return self.emit('error', error);
}

Technically, the way it is currently set up, I think it is outputting the error "[object Object]" just not in a nonusable form.

@vicapow

This comment has been minimized.

Show comment
Hide comment
@vicapow

vicapow Mar 19, 2013

Contributor

@techpines yeah, that would be a better solution. in addition, sails will need to use the LessAsset instead of compiling the less code directly. seems like this code might have been written before asset-rack had support for less.

Contributor

vicapow commented Mar 19, 2013

@techpines yeah, that would be a better solution. in addition, sails will need to use the LessAsset instead of compiling the less code directly. seems like this code might have been written before asset-rack had support for less.

@techpines

This comment has been minimized.

Show comment
Hide comment
@techpines

techpines Mar 19, 2013

Member

@vicapow I did the less one by hand to match how sails was doing the less stuff before. But I don't really like how it concatenates the less files first. I think it does this to handle dependencies, but less supports dependency resolution out of the box with import.

If you got rid of the concatenation step, that would fit more in line with the asset-rack Less asset. Then you wouldn't need the extra custom stuff, plus I think it's just cleaner.

Member

techpines commented Mar 19, 2013

@vicapow I did the less one by hand to match how sails was doing the less stuff before. But I don't really like how it concatenates the less files first. I think it does this to handle dependencies, but less supports dependency resolution out of the box with import.

If you got rid of the concatenation step, that would fit more in line with the asset-rack Less asset. Then you wouldn't need the extra custom stuff, plus I think it's just cleaner.

@vicapow

This comment has been minimized.

Show comment
Hide comment
@vicapow

vicapow Mar 19, 2013

Contributor

i agree. ok. i'm working on a patch right now for asset-rack to produce better less errors then I'll swing back into sails and see if I can remove the concatenation step. sounds good?

Contributor

vicapow commented Mar 19, 2013

i agree. ok. i'm working on a patch right now for asset-rack to produce better less errors then I'll swing back into sails and see if I can remove the concatenation step. sounds good?

@techpines

This comment has been minimized.

Show comment
Hide comment
@techpines

techpines Mar 19, 2013

Member

Yea, sounds awesome!

Member

techpines commented Mar 19, 2013

Yea, sounds awesome!

@vicapow

This comment has been minimized.

Show comment
Hide comment
@vicapow

vicapow Mar 20, 2013

Contributor

ok. so. less compilation is now delegated to asset-rack but this will have to wait until asset-track's version gets bumped and fixes a bug with mimetypes.

once that all happens, sails will get nice less error messages like the following:

Error: Less error: missing closing `}`
    filename: /sails-example/assets/styles/styles.less
    line 7 column 0
    ...
     7: }
    ...
Contributor

vicapow commented Mar 20, 2013

ok. so. less compilation is now delegated to asset-rack but this will have to wait until asset-track's version gets bumped and fixes a bug with mimetypes.

once that all happens, sails will get nice less error messages like the following:

Error: Less error: missing closing `}`
    filename: /sails-example/assets/styles/styles.less
    line 7 column 0
    ...
     7: }
    ...
@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Mar 20, 2013

Member

This is awesome-- saw you just committed- should I wait to pull until Asset Rack is updated? I can change the package.json to point to the master branch of Asset Rack, but I'd feel more confident waiting until there was a stable patch to depend on

Member

mikermcneil commented Mar 20, 2013

This is awesome-- saw you just committed- should I wait to pull until Asset Rack is updated? I can change the package.json to point to the master branch of Asset Rack, but I'd feel more confident waiting until there was a stable patch to depend on

@techpines

This comment has been minimized.

Show comment
Hide comment
@techpines

techpines Mar 20, 2013

Member

@mikermcneil The asset-rack master branch isn't totally clean, don't point sails at it.

I'm going to push something stable to npm tonight, and then I'll come back here with the version number.

Member

techpines commented Mar 20, 2013

@mikermcneil The asset-rack master branch isn't totally clean, don't point sails at it.

I'm going to push something stable to npm tonight, and then I'll come back here with the version number.

@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Mar 20, 2013

Member

Cool, thanks. Bros4lyfe

Member

mikermcneil commented Mar 20, 2013

Cool, thanks. Bros4lyfe

@techpines

This comment has been minimized.

Show comment
Hide comment
@techpines

techpines Mar 21, 2013

Member

New version, enjoy!

asset-rack@2.1.4

@vicapow should update the merge request :)

Member

techpines commented Mar 21, 2013

New version, enjoy!

asset-rack@2.1.4

@vicapow should update the merge request :)

@vicapow

This comment has been minimized.

Show comment
Hide comment
@vicapow

vicapow Mar 21, 2013

Contributor

should be good to go!

Contributor

vicapow commented Mar 21, 2013

should be good to go!

mikermcneil added a commit that referenced this pull request Mar 21, 2013

Merge pull request #244 from vicapow/master
show meaningful less errors

@mikermcneil mikermcneil merged commit 744eea5 into balderdashy:master Mar 21, 2013

@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Mar 21, 2013

Member

Thanks fellas!

Member

mikermcneil commented Mar 21, 2013

Thanks fellas!

@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Mar 22, 2013

Member

Guys, this is breaking my existing projects using LESS. Seems like it breaks the asset loading order (/assets/mixins don't seem to be brought in before /assets/styles)

Member

mikermcneil commented Mar 22, 2013

Guys, this is breaking my existing projects using LESS. Seems like it breaks the asset loading order (/assets/mixins don't seem to be brought in before /assets/styles)

@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Mar 22, 2013

Member

Or it's compiling them independently, so variables in mixins that are included in a previous folder in the asset sequence aren't being made available in the other files

Member

mikermcneil commented Mar 22, 2013

Or it's compiling them independently, so variables in mixins that are included in a previous folder in the asset sequence aren't being made available in the other files

@techpines

This comment has been minimized.

Show comment
Hide comment
@techpines

techpines Mar 22, 2013

Member

I think that's what's happening. It's a breaking change.

Member

techpines commented Mar 22, 2013

I think that's what's happening. It's a breaking change.

@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Mar 22, 2013

Member

Hm- so how do we do mixins? We'll need some way to include lib/mixin LESS files before the rest to have the variables recognized.

Member

mikermcneil commented Mar 22, 2013

Hm- so how do we do mixins? We'll need some way to include lib/mixin LESS files before the rest to have the variables recognized.

@techpines

This comment has been minimized.

Show comment
Hide comment
@techpines

techpines Mar 22, 2013

Member

We could do one of two things:

You can require the mixins:

@import "../../mixins.less";

That is probably how the less folks would recommend doing dependencies.

Or LessAsset could be modified to take some extra parameter that it uses for mixins. Right now the Less Asset looks like this:

new LessAsset({
    url: '/cool-stuff.css'
    filename: './somefile.less'
});

Maybe we could add an option to prepend a less file like this:

new LessAsset({
    url: '/cool-stuff.css',
    filename: './somefile.less',
    prepend: '../mixins.less'
});
Member

techpines commented Mar 22, 2013

We could do one of two things:

You can require the mixins:

@import "../../mixins.less";

That is probably how the less folks would recommend doing dependencies.

Or LessAsset could be modified to take some extra parameter that it uses for mixins. Right now the Less Asset looks like this:

new LessAsset({
    url: '/cool-stuff.css'
    filename: './somefile.less'
});

Maybe we could add an option to prepend a less file like this:

new LessAsset({
    url: '/cool-stuff.css',
    filename: './somefile.less',
    prepend: '../mixins.less'
});
@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Mar 22, 2013

Member

Tried out imports, and it worked for me-- sorry I'm just a LESS n00b, didn't know the best practice there. For anyone else who's curious: http://lesscss.org/#-importing

That said, eventually I think it would still be nice for our LESS modules to understand the variables from previously included LESS files

Member

mikermcneil commented Mar 22, 2013

Tried out imports, and it worked for me-- sorry I'm just a LESS n00b, didn't know the best practice there. For anyone else who's curious: http://lesscss.org/#-importing

That said, eventually I think it would still be nice for our LESS modules to understand the variables from previously included LESS files

@rachaelshaw

This comment has been minimized.

Show comment
Hide comment
@rachaelshaw

rachaelshaw Mar 22, 2013

Member

Thought using imports would fix the problem, but now I'm just getting this:

( to see your app, visit: http://localhost:1337 )  

/Users/rachaelshaw/Desktop/kitten/sails-website/node_modules/sails/node_modules/asset-     
rack/node_modules/less/lib/less/parser.js:421
                    throw new(LessError)(e, env);
                          ^
[object Object]
Member

rachaelshaw commented Mar 22, 2013

Thought using imports would fix the problem, but now I'm just getting this:

( to see your app, visit: http://localhost:1337 )  

/Users/rachaelshaw/Desktop/kitten/sails-website/node_modules/sails/node_modules/asset-     
rack/node_modules/less/lib/less/parser.js:421
                    throw new(LessError)(e, env);
                          ^
[object Object]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment