Add improved source-mapping implementation #792
Conversation
mgreter
changed the title
|
There is still a lot of debug stuff included in this PR which should be removed before merging! |
|
@mgreter, can we test source-maps with sassc without involving ruby/sass-spec? I can write a simple test runner which couts the results. The only "fancy stuff" would be using the json.c and actually parsing the result as JSON and test the member/ingridentes of JSON while treating it as an object (as opposed to string). This way we don't have to worry about the order of items or items in All this in with pure sassc, no ruby, no gem, just C! 😎 |
|
I have added a simple "test runner" to perl-libsass while developing this feature to keep at least some track of what I was doing. @am11 you probably know how hard it is to debug this kind of stuff. I currently (not published yet) store this directory structure as the "sourcemap tests":
The config file is a map to context options (C-API options in a colon separated map), while the rest should be self explanatory. The output.css.map file is parsed and the stuff like included paths should be compared. I currently only implemented to compare the mappings, which does a search for expected mappings only, while ignoring additonal ones! This proved to be pretty effective during developement, since I was able to update the expected file incrementally (after inspecting and verifying the specific cases/mappings manually!). Basically this topic is endless since one could discuss about how a mapping should be done is pretty open (how do you map a calculation? Include every operator, number and so on?). This PR is already at a point where I would say is at least 50% better than before. Some code is certainly not optimal (reparse/remap part), but I don't really know how to improve it much more! And it gets tedious to maintain such a big commit! This particular feature (remap) would not have worked before anyway; so I guess I'm willing to take the risk that it will now only work a few percent better. This might be 100% or 0.05%! I guess not many people really want to try it out and report back specific issues here! So I only can encourage the "first scream" approach! Just get it merged into 3.2 and let's see 🍌 Btw. this inquiry by the original(?) implementor of source-maps seems to be handled here! |
| @@ -140,7 +140,7 @@ extern "C" { | |||
| } | |||
| catch (Sass_Error& e) { | |||
| stringstream msg_stream; | |||
| msg_stream << e.path << ":" << e.position.line << ": " << e.message << endl; | |||
| // msg_stream << e.pstate.path << ":" << e.pstate.line << ": " << e.message << endl; | |||
There was a problem hiding this comment.
Review Note: Revert!
mgreter
force-pushed
the
feature/improved-source-maps
branch
from
33a9e45
to
d31f1ab
Compare
mgreter
changed the title
mgreter
force-pushed
the
feature/improved-source-maps
branch
from
bd615e5
to
e9ef4b4
Compare
Add improved source-mapping implementation
|
Today is turning out to be a big achievement day! 👍 ⭐ Congratulations! :) |
|
WEEEEEEE |
|
Nice work! |
|
Thx all! Just to be perfectly clear, this does not magically fix all of our source-map problems. This is merely a step in the right direction to provide a base to make this happen. I only verified a few common complicated cases. Most of the rest should behave as before plus a few additional perks. |
This PR is the successor of #603 and #786. IMO this is already better than the current implementation, so I dare anyone to try it out. IMO we could include this "as is" in the next version to get this moving in the right direction, since the current implementation does not really work anyway IMHO. The whole feature is very complex, so I cannot give any guarantee that it will work for all mappings. But IMHO the current state of this PR is satisfactory and we could move on from that!