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

[API proposal] Expose add_source() interface #1368

Closed
Tracked by #1892
saper opened this issue Jul 22, 2015 · 9 comments
Closed
Tracked by #1892

[API proposal] Expose add_source() interface #1368

saper opened this issue Jul 22, 2015 · 9 comments

Comments

@saper
Copy link
Member

saper commented Jul 22, 2015

I think it would be pretty good to expose some form of add_source() interface via C-API.

How it would work:

   void sass_add_source(Sass_Context *ctx, const char *import_identifier, const char *contents);
   void sass_add_source_func(Sass_Context *ctx, const char *import_identifier, char *(*func)(...));


   Sass_Context c * = sass_make_context();
   sass_add_source(c, "mixins", "/* ... */");
   sass_add_source(c, "modules/alfa", "/* ... */");
   sass_add_source(c, "modules/beta", "/* beta */");
   sass_add_source(c, "main", "/* first */");
   ....
   sass_add_source_func(c, "dynamic", &import_func());
   ....

   sass_generate(c, "main", output);   

   /* all others stay unchanged */
   sass_add_source(c, "main", "/* second */");
   sass_add_source(c, "modules/beta", "/* beta extended */");
   sass_generate(c, "main", output);

In general libsass core could stop dealing with input files at all (only for compatibility and/or convenience) and just use import identifiers namespace.

It should be possible to replace imported sources on demand. Real custom importer functions could be called on demand.

I could also imagine add_source to be called in asynchronous callbacks after the file is loaded.

add_source() could trigger parsing of the supplied text as well.

@drewwells
Copy link
Contributor

I'm a little confused. How is this different than custom importers?

The biggest performance bottleneck I have experienced is having libsass do file I/O. I had started by reading the Sass files in the implementation, tokenizing/parsing the Sass for imports then sending the concatenated Sass to the data compiler. Switching to the file compiler increased compile time by nearly 50%. It would be nice to have some APIs that made doing file I/O in the implementation easier. I'm not sure how this adds benefits over what the custom importer already makes available though.

@saper
Copy link
Member Author

saper commented Jul 27, 2015

Thank you for this question @drewwells ! I think this actually touches the gist of this issue.

Custom importers work in a pull way and the proposed add_source method is a push way.

Custom importers work this way: libsass processes the file and when it encounters @import statement
it stops the parsing and calls the custom importer routine to provide the data or the file name. After the
response is received, libsass continues processing.

In the new way (which does not remove the possibility to have custom importers as we have now, of course)
you prepare all your bits before starting the execution phase. I actually think that it should be possible
to pre-parse all available resources/mixins and then just invoke the execution engine pointing at the "main" file
(which you may have any). Ideally, if we find a way to nicely persist the libsass context, maybe you could just keep all your files parsed until they change and invoke just the execution engine for (multiple if necessary) "main" files:

`-+ lib
   `-+ mixins
     `--- lib.scss
     `--- cps.scss
     `--- img.scss
   `-+ custom
      `- user.scss
`-+ css
  `--- profilepage.scss
  `--- helppages.scss
  `--- main.scss

After

addsource(c, 'lib/mixins/lib', 'lib/mixins/lib.scss')
addsource(c. 'lib/mixins/cps', 'lib/mixins/cps.scss')
addsource(c. 'lib/mixins/img', 'lib/mixins/img.scss')
addsource(c, 'lib/mixins/lib', 'lib/mixins/lib.scss')
addsource(c, 'css/profilepage', 'css/profilepage.scss')
addsource(c, 'css/helppages, 'css/helppages.scss')
addsource(c, 'css/main, 'css/main.scss')

and then it is possible to do, hopefully without re-parsing everything:

generate(c, 'css/main', 'out/css/main.css')
generate(c, 'css/helppages, 'out/css/helppages.css')
generate(c, 'css/profilepage, 'out/css/profilepage.css')

Actually generate() does not need to touch the file system, it just writes the data to the buffer supplied by the client.

I think the custom importers we have now have been shaped after the Ruby implementation, where the Sass engine does not have a knowledge of files really but it invokes supplied importer (see documentation for Sass::Importers). File system import is just the default importer which can be replaced by another backend, for example database.

Because of the nature of C, it seemed easier to provide a built-in file interface, that complicated the API a bit.

(I have renamed compile to generate in the example at the top).

@mgreter
Copy link
Contributor

mgreter commented Jan 20, 2016

I don't really see a benefit to have a prefilled "virtual filesystem". First thing is that I think that mostly anyone doesn't know beforehand which files are needed, so a on demand loading seems natural after all. Caching of compiled files could also be implemented easily with the existing interface (if wanted at some point, but I think could be something once we have a much more stable and clean code-base).

Any objections to close this one?
Don't see a point to keep these open forever?

@drewwells
Copy link
Contributor

I'm not quite sure I see the benefit of this push method. Like @mgreter said, it's difficult to know which files will be necessary. However, what caching mechanism are you referring to? Is this referring to a custom importer that matches all strings?

For instance,

A.scss --depends on--> a.scss
B.scss --depends on--> a.scss

libSass is called twice here, so it opens a.scss twice rather than using a caching mechanism. I used to parse @import, resolve imports (cache them), then send the final strings along to libSass. When switching to libSass's file compiler, compile times increased by 33%. It would be a significant upgrade to compile times if there was a way to handle file access for libSass to support just such a caching method.

@mgreter
Copy link
Contributor

mgreter commented Jan 20, 2016

The point would be to not call a custom importer for the same file twice, even between different compiles and effectively re-use the already parsed AST tree. This would probably need some kind of check mechanism to see if the source can be served from cache or not. And as I said, this is IMO currently too far away to even consider it. But who knows where this all will lead ...

@drewwells
Copy link
Contributor

Well that does sound far off, since variables could be different depending on what global variables a file needs. Unless those are resolved after AST tree is parsed.

For my use case, it would have been sufficient to allow source notations to control which file/line number is resolved by libSass. The same thing that Chrome does with CSS/JS source which sounds less complex than reusing AST tree between compiles.

ie.

// A.scss
div { p { color: red } }
// a.scss
p { color: black }

Output

// output.css
/* A.scss:line 1 */
div p {
  color: red; }
/* a.scss:line 1 */
p { 
  color: black; }

Then source could be added however you want while preserving sourcemaps and error file/line numbers.

@mgreter
Copy link
Contributor

mgreter commented Jan 20, 2016

The parsed AST tree has a variable node in that case. What you are refereing to is the evaluation stage. We parse the source into an AST "as is". So this could be cached. It's really like php zend does caching with the parsed op tree (which is the equivalent of our ast tree before evaluation and expanding).

@saper
Copy link
Member Author

saper commented Jan 20, 2016

This is exactly what @mgreter means, thank you. I proposed to expose this for two reasons: to let libsass avoid any I/O operations (which breaks node-sass in some cases) and because this function was already there available (it is removed now as far as I can see).

@mgreter mgreter mentioned this issue Jan 22, 2016
12 tasks
@mgreter
Copy link
Contributor

mgreter commented Jan 22, 2016

I created a collective ticket to keep track this and other "non-urgent" feature requests, in order to keep the issue tracker a bit more clean for the more 1st aid issues. I would like to encourage everybody interested in this particular feature to add their comments into this closed issue anyway.

Thanks for your understanding and your contribution!

@mgreter mgreter closed this as completed Jan 22, 2016
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