Update bindings for io.js 3.0.0 #1054
Conversation
saper
force-pushed
the
iojs45
branch
3 times, most recently
from
e7fd87d
to
40d903a
Compare
|
\o/ |
The code has been updated to meet the requirements of the updated V8 engine as provided by io.js. In some cases additional EscapableHandleScope has been provided. SassValueWrapper<T>::New has been changed: in case of instance construction via the function call "sass.types.Color(0,0,0)" instead of "new sass.types.Color(0,0,0)" the "new" part is no longer executed by falling through. This should not be necessary because the constructor is called by the NewInstance call anyway. This fixes the following error: FATAL ERROR: v8::Object::SetAlignedPointerInInternalField() Internal field out of bounds
saper
changed the title
|
It looks like this change is ready now for wider testing and review. The coverage is down by one line in index.js which is interesting, because we no longer throw errors @matryo you might want to have a look at this (I put my comments inline). |
|
Amazing work @saper. I've just come back from my trip. Testing this on OSX now. |
|
Things look good on OSX. I'll give Windows a whirl and start prepping a new release. |
|
|
||
| struct Sass_Context* ctx; | ||
|
|
||
| NanAssignPersistent(ctx_w->result, options->Get(NanNew("result"))->ToObject()); | ||
| ctx_w->result.Reset(Nan::Get(options, Nan::New("result").ToLocalChecked()).ToLocalChecked()->ToObject()); |
There was a problem hiding this comment.
ctx_w->result.Reset(Nan::To<v8::Object>(Nan::Get(options, Nan::New("result").ToLocalChecked()).ToLocalChecked()).ToLocalChecked());
|
So, this is it, I think. I have addressed almost all comments from @kkoopa (big thanks again!). In case I decided to go in the other direction - I have replied to the code review comment in this thread. Every commit in this pull request is standalone and should be passing tests (bar whimsical AppVeyor behaviour), so I will not be squashing them. If we will find problems, I want to have one of those commits clearly identified. I hope I will soon stop whispering |
|
😍 |
|
Looks like AppVeyor has some trouble installing node v0.10.40. |
Use TypeError whenever argument count or type do not match and RangeError if the index is out of bounds.
Update bindings for io.js 3.0.0
xzyfer
added
External - NPM
Bug - Fix
and removed
Dev - Needs Review
External - NPM
labels
Fix one regression with error reporting
Update to io.js 3.0.0, use nan 2.0
The code has been updated to meet
the requirements of the updated V8 engine
as provided by io.js.
In some cases additional
EscapableHandleScopehas been provided.
SassValueWrapper<T>::Newhas been changed:in case of instance construction via
the function call
sass.types.Color(0,0,0)(actually incorrect JavaScript...)
instead of
new sass.types.Color(0,0,0)the
newpart is no longer executed byfalling through. This should not be necessary
because the constructor is called by
the
NewInstancecall anyway.This fixes the following error: