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

Regression: incorrect line numbers in source maps #720

Closed
naartjie opened this issue Dec 11, 2014 · 12 comments
Closed

Regression: incorrect line numbers in source maps #720

naartjie opened this issue Dec 11, 2014 · 12 comments

Comments

@naartjie
Copy link

Hi. It looks like ca5f271 broke something, and line numbers from source maps become out of sync.

I don't know C++ to be able to dig deeper, but I can verify that before this, the line numbers seem to be synced up, at least for my use case, and afterwards they are out. I can also submit my files, so we can create a test case for this. Thoughts?

@mgreter
Copy link
Contributor

mgreter commented Dec 11, 2014

Would be nice if you could show us your example. Best would be a minimal test case! Thank you!

@naartjie
Copy link
Author

Sure, I will put one together. Let me clean up the files I have, and provide as small a test case as possible.

Thanks for looking into this.

@naartjie
Copy link
Author

Here she is, I've tried to strip it down as much as possible:

html {
  padding: 0;
}

body {
  color: #333;
  background-color: #999;
}

@mixin input-size($parent) {
  textarea#{$parent},
  select[multiple]#{$parent} {
    height: auto;
  }
}

@include input-size('.input-sm');

I've also uploaded the compiled examples to this repo:

pre-3.0.1 is compiled with libsass f55b2d5, and post-3.0.1 is compiled with head / 5f3558d.


Oh dear, so after all of that, I've just noticed that the broken map contains an extra ; before: "mappings": ";6BAAA..., and the one that works doesn't have that. If I remove it the line numbers are reported fine.

In my original use case where I discovered this, I had 2 ;; at the beginning of the mappings element, and 2 corresponding @include input-size's, and I just went back now, and it looks like removing the ; fixes the issue. So I hope this is a relatively easy fix ;-).

@nkgm
Copy link

nkgm commented Dec 12, 2014

Same problem here. Problem started with node-sass v1.1.0 - it uses libsass @ ca5f271.

@mgreter
Copy link
Contributor

mgreter commented Dec 12, 2014

Thank you for taking the time to produce a minimal test case!
It surely helps us to get to the root cause faster; but I cannot promise any ETA!

@naartjie
Copy link
Author

@mgreter NP. Thanks for looking into it.

gh-pages finally caught up, and the test cases are viewable in the browser here: broken and working

mgreter added a commit to mgreter/libsass that referenced this issue Dec 19, 2014
@mgreter
Copy link
Contributor

mgreter commented Dec 19, 2014

Maybe someone could try the fix at https://github.com/mgreter/libsass/tree/fix/source-map-issues?
Unfortunately we have no source-map tests, so I'm not sure if this breaks anything else, but IMO it is more correct than before! Maybe we can land this in the next release if somebody can confirm it works.

@naartjie
Copy link
Author

Hi @mgreter. I have tested your fix/source-map-issues branch, and I can confirm this fixes the issue I was having.

Thanks a lot for having a look at this!! Any chance this could make it into the next release? #697

@mgreter
Copy link
Contributor

mgreter commented Dec 22, 2014

Your feedback improved that chance by a lot. Thx! Anyone else!?

@naartjie
Copy link
Author

Sorry it took a while, I had a busy weekend, back on track now ;-)

@mgreter
Copy link
Contributor

mgreter commented Dec 22, 2014

I'm going to close this, since we have merged a possible fix!
Please re-open this only if the problem with leading ; occurs again!

@mgreter mgreter closed this as completed Dec 22, 2014
@naartjie
Copy link
Author

Awesome, thanks @mgreter 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants