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: Drops uv_mutex in favor of std::mutex #615

Merged
merged 2 commits into from
Jan 10, 2015
Merged

Conversation

am11
Copy link
Contributor

@am11 am11 commented Jan 10, 2015

  • Importer: Drops uv_mutex in favor of std::mutex. (0720fa8)
    • It is not safe to use uv_cond_wait
      outside of v8 thread (uv loop).
  • Importer: Hack to make CLI importer usage work. (ff53926)
    • If anyone could ever explain this!
    • This is related to the changes made in 0720fa8.
    • Tidbit: it was also failing on windows with node.js v0.10.32
      updating to v0.10.35 (current stable), it worked without this hack on Windows
      but when tested with v0.10.35 on Ubuntu via nvm, it still fails without this hack.
      The only downside of this hack is extra nuisance on console
      (I tried to dodge it by overriding console.log function, but that didn't work either, so we would have to live with this little annoyance**, until someone can figure out why it is happening).

@browniefed, @xzyfer, can you guys please confirm if this patch makes custom importer useable on Mac (and addresses #586)?


** without the hack running the following in cli hangs indefinitely:

node node_module/.bin/node-sass /path/to/input.scss --importer /path/to/importer.js

with the hack, it executes properly, but prints whatever custom importer returns. Tested on Ubuntu and Windows.

Note 1: If the importer option is not set (in CLI or render() usage, leaving renderSync() alone), no unwanted / extra stuff will not be printed.
Note 2: This means we would have to apply same hack to custom functions feature (#332), whenever we will implement it. :)
Note 3: We use render (as opposed to renderSync) in case of CLI usage, because we route the errors to emitter. In case of renderSync, the errors are thrown to stderr. [1]   [2]

It is not safe to use uv_cond_wait
outside of v8 thread (uv loop).
* If anyone could ever explain this!
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) when pulling ff53926 on am11:std-mutex into 0add991 on sass:master.

@browniefed
Copy link

@am11 hot damn, checked out the std-mutex branch on your fork and it seems to work. Followed the same process on the #586 . Did around 30-40 copy and pastes of

require("./").render({
    data: "@import \"non-existing-file\"; \n.test {\ncolor: #000;\n}",
    outputStyle: 'compressed',
    success: function(result) {
        console.log(result);
    },
    error: function(error) {
        console.log(error);
    },
    importer: function(url, prev, done) {
        done({contents: 'div { color: #000; }'});
    }
});

and they all seemed to work, no errors. Nice!

@am11
Copy link
Contributor Author

am11 commented Jan 10, 2015

Yay! And thanks for testing. 🎉
Could you please test npm test as well?

This will confirm that it's working on all accounts (render, renderSync and CLI usage). :)

@browniefed
Copy link

@am11 664 passing (5s), nothing seems to be failing 👍

@am11
Copy link
Contributor Author

am11 commented Jan 10, 2015

Awesomeness! So its Showtime for v2! :)

Now we need to make the binaries. I have done the windows binaries (need to update the PR). Could you please send a PR for Mac binary on node-sass-binaries repo? Here is a good example.

I can walk you through the steps:

# after this is merged, clone node-sass ... start fresh
git clone https://github.com/sass/node-sass --recursive; cd node-sass
git submodule update --init --recursive; npm install
node-gyp rebuild        # without -d, 
                        # so it should create a folder build/Release (with binding.node)

# on github, fork https://github.com/sass/node-sass-binaries
# then clone your fork, perferablly outside node-sass directory:
cd ..; git clone htttps://github.com/browniefed/node-sass-binaries; cd node-sass-binaries

ls -lhS

rm darwin-x64/*   # just so you can 
cp ../node-sass/bin/Release/binding.node darwin-x64/

ls -lhS   # see the difference in file sizes, so you know you are using latest one,
          # its lame I know just for fun sakes :)

git add .;git commit -am "Binary: Adds Mac binaries for v2.0.0."; git push

And finally the PR. :)

Thanks in anticipation. ⚡ 💯

am11 added a commit that referenced this pull request Jan 10, 2015
 Importer: Drops uv_mutex in favor of std::mutex
@am11 am11 merged commit 97e1563 into sass:master Jan 10, 2015
@am11 am11 deleted the std-mutex branch January 10, 2015 20:11
@am11
Copy link
Contributor Author

am11 commented Jan 10, 2015

Merged! ;)

@browniefed
Copy link

Thanks for the instructions only one small modification node-sass/build instead of node-sass/bin 💯

@callumlocke
Copy link

Curious, does anyone know why the console.log hack works? And does it actually need to print out the whole data object, or could it just print out an empty line?

@am11
Copy link
Contributor Author

am11 commented Feb 18, 2015

@callumlocke, it was explained by @matryo here: #644 (comment). It will be removed when custom functions will arrive to node-sass.

@davidgtonge
Copy link

The console.log hack is rendering this project unusable

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

5 participants