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

[WIP] Improves source map generating (map more tokens) #603

Closed
wants to merge 10 commits into from

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Nov 1, 2014

This PR adds more mappings to the source map.

It seems reasonable to map all kind of tokens back to the original source!

Will create more sophisticated unit tests for perl-libsass (using OCBNET::SourceMap).
Already started to document my findings about source map internals in a wiki page

@am11
Copy link
Contributor

am11 commented Nov 1, 2014

Brilliant! 👍

Is this addressing #324?

@mgreter mgreter self-assigned this Nov 1, 2014
@mgreter mgreter force-pushed the feature/source-map/map-more-tokens branch 14 times, most recently from 12e143e to 7c682a8 Compare November 1, 2014 12:44
@mgreter
Copy link
Contributor Author

mgreter commented Nov 1, 2014

It does not directly adress that issue, but I already changed/fixed some bits there:
7c682a8#diff-24f01ab36b9575fb048acd94dc0abc1fL109
But I have no idea if that coincidentally also fixes this issue!?

@am11
Copy link
Contributor

am11 commented Nov 1, 2014

I will test it once this is merged. Is this ready?

@mgreter
Copy link
Contributor Author

mgreter commented Nov 1, 2014

This is actually far from ready ;) I'm currently in the process to get an overall view of the big picture. There are a lot of nitty gritty details, that have to be considered! Once I get this done, I hopefully can decide on a clean design to have this correctly implemented! The ETA for this WIP is "some day in the future" ;) I mainly did this PR to make my current work transparent to others.

@am11
Copy link
Contributor

am11 commented Nov 1, 2014

Bundle of thanks @mgreter. This work is priceless! 👍

Let me know, if you need my help.. (-8

@mgreter
Copy link
Contributor Author

mgreter commented Nov 2, 2014

I think we need to add some way to make this a collaborating effort ultimately, since it is basically impossible for me to test all variations myself. It boils down to have something like the sass-spec test suite, but for source mappings. It also is more complex than one might think in the first place.

The first findings that I'm pretty sure needs to be adressed, is which informations are stored for AST_Node. Currently it only stores the begining of a "token", but the ending might also be interesting to output. Libsass currently doesn't remember the position where it has found colons or brackets. But these informations are vital to create a more complete mapping! I so far have added the end position of each "Token" to AST_Node. Next on my list is to add some info of what type a mapping is (which should be handy to decide what to render in source maps and also for debugging purposes).

Unfortunately this already gets very verbose in term of code lines added. So I probably have to add some more middleware to get rid of this (which is what I meant by "getting a view of the big picture").

I will try to keep https://github.com/sass/libsass/wiki/Source-Map-Internals updated with my latest findings, but I probably won't have much time to work on this the next weeks!

mgreter and others added 8 commits November 3, 2014 20:20
Used to generate mapping for source maps.

Conflicts:
	.travis.yml
	Makefile
	Makefile.am
	ast.hpp
	eval.cpp
	extend.cpp
	parser.cpp
	script/cibuild
	script/install-compiler
Currently used to identify who rendered a mapping.

Conflicts:
	.travis.yml
	ast.hpp
```
foo($bar...);
```

Will flag `$bar` as a variable arg when in fact it may be a keyword
arg. We can only be sure once `$bar` has been evaluated. If `$bar`
is evaluated to a map then it's a keyword argument and the
Argument object needs to be updated accordingly.

Conflicts:
	eval.cpp
@mgreter mgreter force-pushed the feature/source-map/map-more-tokens branch from 3096d50 to af98e89 Compare November 3, 2014 19:48
@mgreter
Copy link
Contributor Author

mgreter commented Nov 3, 2014

I'm making some progress here. Had to add some status information to AST_Nodes just for debugging (can probably be removed once I figured out how exactly I want to add the mappings). Currently I'm figuring out how libsass generates it's output to get an idea how I could appropriately implement it cleanly. For this I'm using a perl module I had laying around to read the source-map json and generate some debug information. This is what it currently produces:

SCSS Input

@media (max-width: 600px) { A { color: foobar; } }

CSS Input

@media (max-width: 600px) {
  A {
    color: foobar; }
 }

/*# sourceMappingURL=test.map */

Source Map Debug

0:0:0:0:0:310 => Statement* Expand -> Media_Block (open)
  0:7:0:0:0:104 => Expression* Eval::operator()(List* l) -> List (open)
    0:7:0:0:0:7329 => Expression* Eval::operator()(Media_Query* q) -> Media_Query (open)
      0:7:0:0:8:7330 => Expression* Eval::operator()(Media_Query_Expression* e) -> Media_Query_Expression (open)
        0:8:0:0:8:854 => Expression* Parser -> String_Constant (open)
        0:17:0:0:17:10854 => Expression* Parser -> String_Constant (close)
        0:19:0:0:19:7325 => Expression* Eval::operator()(Textual* t) -> Number (open)
        0:24:0:0:24:17325 => Expression* Eval::operator()(Textual* t) -> Number (close)
      0:25:0:0:25:17330 => Expression* Eval::operator()(Media_Query_Expression* e) -> Media_Query_Expression (close)
    0:25:0:0:6:17329 => Expression* Eval::operator()(Media_Query* q) -> Media_Query (close)
  0:25:0:0:6:10104 => Expression* Eval::operator()(List* l) -> List (open)
  1:2:0:0:26:3106 => Expression* cval_to_astnode -> Map (open)
    1:2:0:0:0:717 => Selector_List* Parser::parse_selector_group() -> Complex_Selector (open)
      1:2:0:0:0:801 => Complex_Selector* Parser::parse_selector_combination() -> Complex_Selector (open)
        1:2:0:0:26:802 => Compound_Selector* Parser::parse_simple_selector_sequence() -> Compound_Selector (open)
          1:2:0:0:0:805 => Compound_Selector* Parser::parse_simple_selector_sequence() -> Type_Selector (open)
          1:3:0:0:1:10805 => Compound_Selector* Parser::parse_simple_selector_sequence() -> Type_Selector (close)
        1:3:0:0:27:10802 => Compound_Selector* Parser::parse_simple_selector_sequence() -> Compound_Selector (close)
      1:3:0:0:1:10801 => Complex_Selector* Parser::parse_selector_combination() -> Complex_Selector (close)
    1:3:0:0:1:10717 => Selector_List* Parser::parse_selector_group() -> Complex_Selector (close)
  1:3:-1:-1:-1:13106 => Expression* cval_to_astnode -> Map (close)
  1:3:0:0:30:123 => Statement* Expand::operator()(Block* b) -> Block (open)
    2:4:0:0:32:667 => Statement* Expand::operator()(Declaration* d) -> Declaration
      2:4:0:0:32:835 => Declaration* Parser::parse_declaration() -> String_Constant (open)
      2:9:0:0:37:10835 => Declaration* Parser::parse_declaration() -> String_Constant (close)
      2:9:0:0:37:837 => Declaration* Parser::parse_declaration() -> String_Constant (open)
      2:11:0:0:38:10837 => Declaration* Parser::parse_declaration() -> String_Constant (close)
      2:11:0:0:39:854 => Expression* Parser::parse_value() -> String_Constant (open)
      2:17:0:0:45:10854 => Expression* Parser::parse_value() -> String_Constant (open)
    2:17:0:0:45:10667 => Statement* Expand::operator()(Declaration* d) -> Declaration (close)
  2:20:0:0:48:10123 => Statement* Expand::operator()(Block* b) -> Block (close)
3:2:0:0:50:10123 => should be 10310 ????

There are still a lot of issues, but IMO it's already a start. Thought I'd share that here :)

@am11
Copy link
Contributor

am11 commented Nov 3, 2014

Super fabulous IMO! 👍

Thanks for sharing! :)

@mgreter mgreter force-pushed the feature/source-map/map-more-tokens branch 3 times, most recently from 78b925b to 941ddd6 Compare November 4, 2014 05:05
@mgreter mgreter force-pushed the feature/source-map/map-more-tokens branch from 941ddd6 to 3666134 Compare November 4, 2014 05:26
@mgreter
Copy link
Contributor Author

mgreter commented Nov 4, 2014

Hehe, yeah, that is how my default editor is setup, I actually like smarttabs and allman style ;)
Tabs! Spaces! Both!
I will pretty sure release this as a standalone library, since I want to use it to speed up my perl source map library too. And it may also be handy if we want to test source-maps via sassc!

@am11
Copy link
Contributor

am11 commented Nov 4, 2014

LOL that is funny. 😄

I guess two-tab indentation is a convention in libsass project.

@HamptonMakes
Copy link
Member

soft tabbbbbssssss

On Tue, Nov 4, 2014 at 5:45 PM, Adeel Mujahid notifications@github.com
wrote:

LOL that is funny. 😄

I guess two-tab indentation is a convention in libsass project.

Reply to this email directly or view it on GitHub:
#603 (comment)

@nschonni
Copy link
Collaborator

nschonni commented Nov 5, 2014

@mgreter this library has an EditorConfig file https://github.com/sass/libsass/blob/master/.editorconfig
See if your IDE has a plug-in and then you don't have to think about it anymore

@am11
Copy link
Contributor

am11 commented Nov 17, 2014

@mgreter, would this be merged before libsass vNext is out?

@mgreter
Copy link
Contributor Author

mgreter commented Nov 17, 2014

No, sorry, I really don't think so. This needs a lot more work!

@mgreter
Copy link
Contributor Author

mgreter commented Dec 29, 2014

I have a second WIP branch which tackles this problems from another side.
This fixes source-maps when selectors get expanded:
https://github.com/mgreter/libsass/tree/source-map-test

That Branch already seems to produce better results than the current implementation, but it needs a lot more re-factoring and better testing (which I have started with the help of perl-libsass).

@mgreter
Copy link
Contributor Author

mgreter commented Dec 31, 2014

Closing in favor of #792!

@mgreter mgreter closed this Dec 31, 2014
@mgreter mgreter removed this from the 3.3 milestone Mar 9, 2015
@mgreter mgreter deleted the feature/source-map/map-more-tokens branch April 6, 2015 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants