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

Importer: General overhauling #589

Merged
merged 6 commits into from
Jan 6, 2015
Merged

Importer: General overhauling #589

merged 6 commits into from
Jan 6, 2015

Conversation

am11
Copy link
Contributor

@am11 am11 commented Dec 28, 2014

  • Fixes the async crashing by explicitly
    closing async.
  • Changes the behavior of renderSync importer
  • Updates index accordingly.
  • Removes obsolete tests accordingly.

BIG THANKS to @txdv and this addresses #586 ! 🎉👍

Need to update the docs accordingly.


Code: Adds NanScope() to guard nan handlers.

  • Fixes for Persistent.
  • Credit @kkoopa (thanks for pointing these out!).

@am11 am11 changed the title Importer: General overhauling.. 🐛🪲 Importer: General overhauling Dec 28, 2014

if (importer) {
options.importer = function(file, prev, key) {
var done = function(data) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function done(data)

@am11 am11 force-pushed the master branch 2 times, most recently from 4e475c9 to 4aee39b Compare January 1, 2015 12:01
@am11
Copy link
Contributor Author

am11 commented Jan 1, 2015

@kevva, ignoring the fact that we are getting segmentation fault, there is another (*nix-only) issue with cli importer tests, timeout.

So I separated the cli command. With my master branch (and libsass submodule pointing to their current master), in node-sass clone root dir, run the following in bash:

node bin/node-sass test/fixtures/include-files/index.scss --output test/fixtures/include-files
--importer test/fixtures/extras/my_custom_importer_file_and_data_cb.js

It would not produce any output. But if console.log('blah') statement is added in lib/index.js#render() function (or in importer function inside my_custom_importer_file_and_data_cb.js), it generates the output and prints chalked message on the screen. Could this be due to any dependency, emitter and so forth?

@am11 am11 force-pushed the master branch 2 times, most recently from c9669b6 to fa44d56 Compare January 1, 2015 14:14
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ce4b489 on am11:master into * on sass:master*.

@am11
Copy link
Contributor Author

am11 commented Jan 1, 2015

Turned out this CLI issue is ultimately related to binding. Only happening on Linux. On Win, it is passing all the tests.

I commented out free() statement to confirm if all tests are passing on TravisCI and yet they are. Nothing new there. I have reverted it back.

Now, lets wait for libsass to release 3.1.0, and I will update the submodules again.

* Fixes the async crashing by explicitly
  closing async.
* Changes the behavior of renderSync importer
* Updates index accordingly.
* Removes obsolete tests accordingly.
BIG THANKS to @txdv and this addresses sass#586 ! 🎉👍
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling fc1d4e6 on am11:master into * on sass:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 84fe3fe on am11:master into * on sass:master*.

@am11
Copy link
Contributor Author

am11 commented Jan 5, 2015

TravisCI says its all green! 🌴

Finally! yay!!
🎉

@mgreter
Copy link
Contributor

mgreter commented Jan 5, 2015

👍 Without known memory leaks now? If yes, what was the problem?

@am11
Copy link
Contributor Author

am11 commented Jan 5, 2015

One memory concern (I don't know if it is really a leak): in case of RenderSync or RenderFileSync, if I call free(ctx_w) it throws SIGSEGV. So I disposed everything else inside ctx_w and otherwise, except for the ctx_w itself. Why I don't think if it is a leak, because NanScope() macro seems to do some magic there and (it feels like -- not sure) it tries to dispose ctx_w. Maybe I am totally wrong and may be there still exist a way to free the ctx_w without segfaut (something like running uv_walk on all handles over and over until we are definitely sure all handles are released then free). At this point it seems pretty trivial. This was the major show stopper.

Then earlier I forgot to uv_close the async. Also, the invalid instruction exception I mentioned on sassc commit (happening occasionaly) was I think due to the reason that we were setting file_path in both file can data context, so I put the condition to prevent it from setting in case of file context (since we are already passing file path in sass_make_file_context). After that, I didn't get that in a while. Need to do some stress testings to be 100% sure: like running require("./").renderSync({data:"a{foo:1}"}) in while(true) loop etc.)

Since there is always room for improvement, for now I guess its fair enough.

I can still reproduce that CLI issue that I mentioned above, but @equalsraf via IRC told me that he is unable to reproduce it and TravisCI confirms that! (don't know if it is Ubuntu or just my bad luck!)

Thanks to everybody who helped us getting here. :)

@am11
Copy link
Contributor Author

am11 commented Jan 5, 2015

(while(true) was a bad idea after all, had to terminate the VM 😜 )

am11 added a commit that referenced this pull request Jan 6, 2015
Importer: General overhauling
@am11 am11 merged commit a956d18 into sass:master Jan 6, 2015
@am11 am11 mentioned this pull request Jan 8, 2015
3 tasks
Friendly-users referenced this pull request in Friendly-users/node-sass Jul 9, 2024
-----
It is inappropriate to include political and offensive content in public code repositories.

Public code repositories should be neutral spaces for collaboration and community, free from personal or political views that could alienate or discriminate against others. Political content, especially that which targets or disparages minority groups, can be harmful and divisive. It can make people feel unwelcome and unsafe, and it can create a hostile work environment.

Please refrain from adding such content to public code repositories.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants