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

Pass the compiler to custom functions instead of the options. #1227

Closed
wants to merge 1 commit into from
Closed

Pass the compiler to custom functions instead of the options. #1227

wants to merge 1 commit into from

Conversation

tristanlins
Copy link
Contributor

For me it makes a lot more sense to pass the compiler to custom functions, so how is it done also in imports.
With the compiler I can not only access the options, I can access the context and the import stack.
Especially the import stack is very interesting, e.g. when you need to generate relative URLs in a custom function.

@xzyfer
Copy link
Contributor

xzyfer commented May 20, 2015

Thanks for the suggestion @tristanlins.

This probably affects @chriseppstein and @drewwells the most. Would also like your feels here @mgreter.

@xzyfer xzyfer added the C API label May 20, 2015
@tristanlins
Copy link
Contributor Author

While testing this, I find out that this wont work.
The @import statements are evaluated before any custom function. In result the import stack is always empty when the custom functions are called.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 80.28% when pulling 628ea47 on tristanlins:feature/custom-function-arguments into 221d7fb on sass:master.

@xzyfer
Copy link
Contributor

xzyfer commented May 20, 2015

@tristanlins is a dead end then? Should this issue be closed?

@saper
Copy link
Member

saper commented May 21, 2015

Probably we should identify the state of the executor vs compiler's state. We should be able to provide a stable interface to the custom function implementors (for example, I'd like to have it serializable #1108). Even if some reflective functions should be made available ( e.g. #1065 ) we may need to provide a stable interface there as well.

Not only custom functions are affected - probably having the same interface for the built-ins would be beneficial as well.

@drewwells
Copy link
Contributor

Maybe detailing what use cases this solves would help illuminate why this is necessary. I'm a big believer in only exposing the amount of information needed to extensions to keep them sane. For instance, I do not need to keep track of where I am in the import stack and I don't see a reason custom functions would need that information. The best use case for that is throwing coherent errors. If a custom function returns an error SassValue, libsass locates the error and builds a stack trace for me. Not something I'd care to do myself.

@tristanlins
Copy link
Contributor Author

and I don't see a reason custom functions would need that information.

We need this information because we need to calculate relative paths. Not from the file itself, but from the context the file is defined in. And to know the context, we need to know which file calls the function.
Its a bit complex, because we deliver "virtual" resources with out application and we have a function very similar to the *-url compass functions. Expect that our urls depend on the file's context, not a global definable location... it's a lot of magic ;-D

@drewwells
Copy link
Contributor

Perhaps there's an alternative way of doing this. https://github.com/wellington/wellington#functions supports all the compass image operations. Just like Compass, file i/o calls like sprite-map('../') are based on global configuration, not the sass file in which the function is called. This makes Sass refactoring very simple, since the function calls are independent of the context in which they are called.

It's not entirely clear to me if this level of context information is available to custom functions in Ruby Sass http://sass-lang.com/documentation/Sass/Script/Functions/EvaluationContext.html. It looks like you may able to derive the current template from the http://sass-lang.com/documentation/Sass/Engine.html options passed via the Environment to a Ruby Custom Function.

However, storing files relative to sass is not something I see often. Web projects, especially as they grow, organize similar resources into distinct directories ie. images/, fonts/, or node_modules/eye-glass-module/.

Thoughts?

@chriseppstein
Copy link
Contributor

Here's a couple of use cases:

  1. sass-based configuration. A sass file should be able to be used to configure a project instead of using an external configuration file. This can only work if you have some concept of where that sass configuration file is on the filesystem.
  2. API scoping. In eyeglass, a sass file that is in an NPM module can access the sass files and assets for only those modules for which is has a direct dependency. We are able to control that access for importing because we are given the current sass file that is doing the import, but when functions try to access an asset, I cannot tell whether the asset is accessible to the current sass file unless I know what npm module it is in.

@saper
Copy link
Member

saper commented May 21, 2015

I see that lots of use cases revolve around mapping "import-identifier" to "actual-imported-resource"

So, in general, what is needed is an array of:

  ('@identifier', 'string')

or

  ('@identifier', sourcefile)

mappings?

Second question:

What about introducing a way to specify such mapping upfront to libsass, something like:

  ctx = sass_new_context();
  sass_add_file_source(ctx, 'a', 'sub/a.scss')
  sass_add_file_source(ctx, 'b', 'sub/b.scss')
  sass_add_subtree_source(ctx, 'mixins', 'proj/mixins') /* for subdirectories */
  sass_add_data_source(ctx, 'c', '/* ... */')
  sass_add_custom_source(ctx, 'special', custom_import_func)
  sass_compile(ctx);

This might actually reduce the need for custom importers - as those records could be added dynamically upfront before rendering.

xzyfer added a commit to xzyfer/sass-spec that referenced this pull request Jun 29, 2015
@mgreter mgreter mentioned this pull request Jul 7, 2015
10 tasks
@mgreter
Copy link
Contributor

mgreter commented Jul 16, 2015

Closing this. We are moving in the direction to have more context information available on the C-API. But we need to take step by step. We already have other open issues regarding similar issues, so this PR serve no real use anymore. Exposing the import context is probably one of the next things to do ...

@mgreter mgreter closed this Jul 16, 2015
@tristanlins tristanlins deleted the feature/custom-function-arguments branch August 3, 2015 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants