-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
This is looking really nice, I'm not going to have chance to properly review for a few days but maybe @nschonni or @LaurentGoderre can weigh in as well. |
👍 I like normalizing the options in one place. |
Tagging this as a 0.9 since it will require a full set of new binaries |
Ok, all tests are running and the pull-request is ready for review. The stats-object now looks like this: {
entry: "path/to/entry.scss", // or just "data" if the source was not a file
start: 10000000, // timestamp
end: 10000001,
duration: 1, // end - start
includedFiles: [ ... ], // absolute paths to all related scss files
sourceMap: "..." // the source map string or null
} Check out I've put the If you're ok with these changes I'll add them to the readme and the pull-request is ready to merge. One thing I noticed: The |
Needs rebasing, if you could please be so kind @jhnns |
… of re-creating it
I've rebased the pull-request. The tests are still running on node 0.10, but they break on node 0.11.11 due to latest changes in node core. According to the nan changelog we need to update nan to Can you review the pull-request and merge? |
💯 a separate PR makes sense
@andrew do you want to do a new release, I tagged this as 0.9 since the API is changing but maybe you're ok with this in a point release since it is under v1 |
Yeah I don't mind a new point release, will try and get a new release out today with this in. |
Feature: Provide included files
Yay! 👍 |
Just trying to work out why the tests are passing on travis but not on my mac, then we'll need some new binaries, cc @LaurentGoderre |
I don't have the time to debug this today, but this isn't working at all on the mac, returning |
Do you have a stack trace? |
It doesn't thrown any kind of stack trace, is there a flag I need to pass or debugger I need to use? |
No, i don't know any flag. Can you identify which function is undefined when Unfortunately I don't have a mac and hence can't reproduce it. |
Could you check if #302 fixes this problem? |
@jhnns here's the failing test output: https://gist.github.com/andrew/387ac9d2350d4b3d9009 |
Sorry ignore all that @jhnns, turns out I wasn't correctly rebuilding the binary, it's all good now. Will get a new release out once we have all updated binaries ready in https://github.com/andrew/node-sass-binaries |
I got the same issues with |
Hi, this is my pull request as discussed in #279. Please note that this pull request is not finished yet, see below.
I was working on this feature the past days when I realized that the code would get messier if I'd just pass another argument to bindings.render() (which has already seven arguments). Additionally I found it hard to grasp with optional arguments. That's why I decided to refactor the binding so it accesses the options object via property names instead of array indices.
Furthermore I noticed that the newly introduced
renderFile
(#285) doesn't normalize the passed-in options. Since this doesn't seem to be intended to me I decided to normalize them too.As you can see I just refactored the code without touching the tests. Now all tests are running again, so this is currently save to merge back into the master (to prevent merge conflicts). Just wanted to get early feedback if you're ok with my refactoring.
Could you also check the C-code because I'm not an experienced C-programmer and might have missed something.