-
Notifications
You must be signed in to change notification settings - Fork 133
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
ArgumentError - invalid byte sequence in UTF-8 #114
Comments
Note that this error seems to only occur when the version of |
Can you prepare minimal app to reproduce this? |
Sure. I'll give it a go. |
I've got that. Working on a fix. |
Sweet. Thanks! |
Can you test this? gem 'less-rails', github: 'metaskills/less-rails', branch: 'depends_only_on_less_files' |
|
Wait... ignore that last. |
@simi Thanks for your excellent work (and ultra-fast response) on this. Thanks again! |
I'm not actively using less-rails now in my projects, so I'm not sure if this is not breaking anything. Do you think your project is big enough to confirm that? Or do you have any chance to test this fix in some bigger project? It will be nice to also prepare some less-rails test project to ensure it is not breaking things. Also I'm not sure how big this change is. Do we need patch, minor or major bump? @metaskills @maxd any ideas? |
Well my project is big, but it doesn't use less that much (it uses scss much more). So I'm not sure it'd be the best to go by. That said, the fix works perfectly. |
@simi, Idea: as a compromise, could this change be merged but as an "experimental setting" that is off by default? So, for instance, this new code would only come into use if I ran something like the following (presumably in an initializer):
I would be perfectly amenable to this (especially on a trial basis while we establish if the code is stable) because my only concern is that I don't want to be fetching code that isn't going to be maintained with bug/security updates. Also, this way, this fix is easily implementable for anyone else experiencing this issue. Finally, my 2 cents (for what it's worth): if you go this route, since the change doesn't affect default functionality and won't change anything for users currently using the gem, I'd just bump the revision part of the version. |
Testing this out too now. Works so far. Thanks! |
hey @simi I've got a 37 kloc less rails project consisting of extensive @imports including one from bower component supplied less file. And I'm actively developing it and will for sure for at least half a year to come. If you can consider this a decent project, I can switch less-rails version for testing and provide feedback. But first we need to merge #116 :) because before that I need my branch to be active :) |
@ilyapoz merged! |
Ok, so now feel free to invite me for testing as promised. So what branch are we talking about here? depends_only_on_less_files? |
Hey there, what's the state of the |
this is merged in less-rails? |
+1, seems like using |
Had the same issue, the |
I only have one .less file in my project, so I'm not sure its not breaking anything either. |
ℹ️ no activity for years, closing |
I'm getting the following error when
rails
is attempting to precompile a stylesheet.Upon further analysis, it seems the issue is that
less-rails
is resolving a css@import
directive to refer to a image asset in myrails
app rather than another stylesheet. Obviously, trying to parse a binary image astext/css
explains this error.The including CSS (really an
.scss
) file in question is innocuous enough. The error is thrown on a line containing:@import 'theme/responsive';
and the file is located atapp/assets/stylesheets/contrib/theme.scss
.In my debugging, I inserted a breakpoint here and evaluated the following code to receive these values:
I'm not too familiar with how this gem works/is supposed to work, but it would seem to me that
app/assets/images
shouldn't be searched for CSS/SCSS/LESS imports at all.Note that this issue was solved (once I deciphered the source of the error from the cryptic message) by changing
@import 'theme/responsive';
to use a relative path:@import '../theme/responsive';
, which perhaps is the better way to go regardless, but that is beside the point that the former should have been supported (or at least returned a more descriptive error).The text was updated successfully, but these errors were encountered: