Replace util._extend with Object.assign

[nschonni]

This was never part of the public API, and has officially been depreciated in Node 6 nodejs/node#4903

[xzyfer]

I don't thing Object.assign has support in older versions of node. We already depend on lodash so we can use lodash.merge.

[nschonni]

You're right, it's an ES2015 thing https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign
We don't actually pull in the whole lodash project, but I could look at adding that part, and replacing the "object-merge" dep in the specs stuff too

[nschonni]

OK, tried to use the lodash.assign. If you think the merge behaviour http://stackoverflow.com/a/33247597/455535 makes more sense I'll swap it.

[xzyfer]

I don't understand what this code is doing. Shouldn't it just be the following?

throw new Error(JSON.parse(result.error))

[xzyfer]

> var result = {error: '{"message": "Oops I did it again"}'};
< undefined
> var err = Object.assign(new Error(), JSON.parse(result.error));
< undefined
> err
< Error: Oops I did it again(…)

[xzyfer]

Maybe @am11 can shed some light on why this approach was taken in 50f374d?

[am11]

Yup, the idea was to have the error object thrown (or return in case of asynchornous call) to the caller. We tried to avoid JSON parsing on native side and casting it to object in JavaScript using util._extend (without having to know what is in the object, line/column/message).

lodash.assign approach is the best backward compatible solution, if we are already depending on that lib.

[xzyfer]

I'm not following what the difference is between this and throwing an error
as I posted earlier?

On 8 Sep 2016 9:24 PM, "Adeel Mujahid" [email protected] wrote:

In lib/index.js
#1711 (comment):

@@ -437,7 +437,7 @@ module.exports.renderSync = function(opts) {
return result;
}

• throw util._extend(new Error(), JSON.parse(result.error));
• throw assign(new Error(), JSON.parse(result.error));

Yup, the idea was to have the error object thrown (or return in case of
asynchornous call) to the caller. We tried to avoid JSON parsing on native
side and casting it to object in JavaScript using util._extend (without
having to know what is in the object, line/column/message).

lodash.assign approach is the best backward compatible solution, if we
are already depending on that lib.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/sass/node-sass/pull/1711/files/61f42dee63a8175cf2ccaa35e41ee243b3875074#r77988340,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAjZWCcVzPeaZpxPQDIx4h0mJ49uRhHhks5qn_B2gaJpZM4J2hC7
.

[am11]

throw new Error(JSON.parse(result.error)) in this case throws Error: [object Object] instead of intended { Error: Oops I did it again.

[xzyfer]

Right, so result.error is a json string. I see. Thank.you.

[am11]

Yup, the reason we are returning string is because libsass returns us string (via sass_context_get_error_json) and we would need to parse the JSON in C++ (v8 binding).

Now thinking about it more, there are two approaches we can avoid this new Error()+ loadash.assign() overhead in JS code:

1. use sass_context_get_error_[message/line/column] functions and construct the Error typed object in C++. I used those in LibSass.NET and wrote some basic tests around them and they work (i.e. no latent bugs lurking in those API functions).
2. keep using sass_context_get_error_json from LibSass and use v8::JSON::Parse (which I happen to just discovered) to create an Error typed object in C++.

I would go with option 1 as LibSass already has those constituents available in memory and probably even lesser overhead constructing an Error object and assign values than constructing and parsing the JSON values in C++.

This is me just being super pedantic, JSON.parse in JS is sufficient and jsonParse+lodashAssign is probably not incurring a significant overhead; but doing it on C++ side would render errors bit faster in stress testing (not in real world scenarios though). If you guys think we should greedily save some cycles and generally care for such [in]significant optimization oppertunities, I can make a patch over the weekend on top of this PR. Otherwise, this SGTM. 😸

[xzyfer]

As you say, I'm not concern with burning a couple cycle to produce an error that will kill the process anyway :)