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 bug: Preserve include paths which were pushed to linked list #1871

Closed
am11 opened this issue Jan 17, 2016 · 2 comments
Closed

API bug: Preserve include paths which were pushed to linked list #1871

am11 opened this issue Jan 17, 2016 · 2 comments

Comments

@am11
Copy link
Contributor

am11 commented Jan 17, 2016

At present, consumers have two choices to set include paths: sass_option_set_include_path and sass_option_push_include_path.

The first one requires a string with concatenated paths, joined by path separator. LibSass internally split the string with platform-aware path separators.

Most consumers, including node-sass, are using this approach and it works.

The second one lets the consumer to insert one path at a time to internal include_paths linked list.

With second approach, the paths are pushed correctly, however, parser ignores those paths. This is because when cpp_ctx is instantiated, the existing include_paths are not copied from the original context. See https://github.com/sass/libsass/blob/1c92e8f/src/context.cpp#L95 (the line is commented out).

IMO. this can either be fixed in the existing form or sass_option_push_include_path should be removed from API surface area and docs. :)

@mgreter
Copy link
Contributor

mgreter commented Jan 18, 2016

This was disabled in ea57348, but I don't remember why ...

@am11 are you sure it works the way you're writing? I'm not sure if include_paths and sass_option_push_include_path are related. I think that include_paths was only available for the obsolete interface and that I replaced it with sass_option_push_include_path on the new context api?

@am11
Copy link
Contributor Author

am11 commented Jan 18, 2016

I am setting it like this: https://github.com/am11/libsass-net/blob/72dde69/src/SafeSassContextHandle.cs#L182-L188.

I also debugged the code around include_path[s] and found that sass_option_push_include_path does get called from different code paths, but strings inserted into linked list before compile() are ignored during the compilation. It seems like options->include_paths and context->include_paths are redundant and the one in options is not copied in context ctor (and hence remain unused).

If possible, can we unify both and have one include_paths linked list (which gets flattened as array later as well; i.e. another overhead)?

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

3 participants