node-sass
node-sass copied to clipboard
sass
Convert binding to N-API
Heavy work in progress of replacing NAN with N-API.
Attempting to combine the prior work https://github.com/boingoing/node-sass/pull/1 with the drift in master, namely #2128 #2298.
Fixes #1988
This is getting pretty close. There's a segfault in one of the tests I still need to debug.
It's worth noting this unwinds the work @stefanpenner's to implement Nan::ObjectWrap (#2128) largely because I couldn't get my head around how to make Napi::ObjectWrap<T> work.
This potentially unwinds @kkoopa's work to propagate async context (#2295) because I couldn't find the equivalent in N-API.
I have it compiling and running most of the tests. There is a segfault that seems semi random. I'll need to investigate.
I've update the the install scripts to use the N-API version instead of module version. Have confirmed binary compiled Node 10 executes on Node 6, 8, 9, 10. Although 9 displays an annoying warning that people will definitely files issues about because...
I've resolved the segfault. All tests are passing locally (OSX) on all Node LTS and current.
We'll need to rejig CI a bit since the binary no longer builds on Node < 10.
We have our first green N-API builds https://travis-ci.org/sass/node-sass/builds/404487187
Just for kicks the current hacked CI setup shows that binaries built on Node 10 work on other LTS Node.
No, the node-pre-gyp handles napi vs the module version in it's download and fallback
Sorry what I mean is that with N-API we can no longer compile the binary on Node < 10. So if the download fails there's no point node-pre-gyp falling back to build locally.
Ah, gotcha, but I think it is backported to v6 & v8. I think older stuff may be able to have support through https://github.com/nodejs/node-addon-api/blob/master/doc/setup.md but I'm not :100:
In my experiments binaries built on 10 run back until 6, but I couldn't get it to build on < 10 because the have different napi versions. I could be missing something though
On Tue., 17 Jul. 2018, 2:14 am Nick Schonning, [email protected] wrote:
Ah, gotcha, but I think it is backported to v6 & v8. I think older stuff may be able to have support through https://github.com/nodejs/node-addon-api/blob/master/doc/setup.md but I'm not 💯
— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/sass/node-sass/pull/2440#issuecomment-405301642, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWEVhL0lhyjPYXpGzE9DtAqzfK9nUks5uHLvogaJpZM4VGwiO .
I have a call with some napi folk later in the week. I'll get some clarification
On Tue., 17 Jul. 2018, 2:29 am Michael Mifsud, [email protected] wrote:
In my experiments binaries built on 10 run back until 6, but I couldn't get it to build on < 10 because the have different napi versions. I could be missing something though
On Tue., 17 Jul. 2018, 2:14 am Nick Schonning, [email protected] wrote:
Ah, gotcha, but I think it is backported to v6 & v8. I think older stuff may be able to have support through https://github.com/nodejs/node-addon-api/blob/master/doc/setup.md but I'm not 💯
— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/sass/node-sass/pull/2440#issuecomment-405301642, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWEVhL0lhyjPYXpGzE9DtAqzfK9nUks5uHLvogaJpZM4VGwiO .
It's worth noting this unwinds the work @stefanpenner's to implement Nan::ObjectWrap (#2128) largely because I couldn't get my head around how to make Napi::ObjectWrap<T> work.
As long as the memory leak scripts I wrote, don't demonstrate a leak we should be ok
@xzyfer I can try and find some time later this week, to see if this regresses any of the issues I saw earlier. Let me know if I should.
Switching to the napi is nice (Assuming supported nodes support the needed napi's), as it is much nicer then NaN. But we should be sure not to regress on those leaks.
As long as the memory leak scripts I wrote, don't demonstrate a leak we should be ok
I can try running them locally as a verification.
I can try and find some time later this week, to see if this regresses any of the issues I saw earlier. Let me know if I should.
This would be great. I'm not planning to change the N-API implementation at the moment.
Assuming supported nodes support the needed napi's
It supports our reduced Node support as of v5 which is where I'd hope to land this.
Sorry what I mean is that with N-API we can no longer compile the binary on Node < 10
@nschonni looks like I was wrong about this. I must have something weird in my local environment. Node 6-10 compile fine in CI.
Looking at https://nodejs.org/api/n-api.html#n_api_n_api_version_matrix, I think the minimum version supported has to be raised to 6.14.
Maybe TravisCI should be adjusted to test on the minimum supported version of each LTS line?
Hey @realityking, a couple updates from today
- I had call with some of the N-API folks this morning, and
- had a chat with @nschonni
It's now my expectation that we'll only rollout the N-API binding to Node 10+. Requiring minimum Node versions is simply too fragile. So moving forward we will be using nan for Node < 10 and N-API for Node 10+.
That's very interesting
Requiring minimum Node versions is simply too fragile
Is that because npm doesn't bail out when the the minimum node version isn't met, instead just printing a warning?
Partly, but largely because the typical user of node-sass either isn't in control of their node version or doesn't know how/why to change versions.
It also introduces another barrier to adoption. We're really focused on making the installation process more forgiving. It's the single largest source of issues.
On Fri., 20 Jul. 2018, 5:20 pm Rouven Weßling, [email protected] wrote:
That's very interesting
Requiring minimum Node versions is simply too fragile
Is that because npm doesn't bail out when the the minimum node version isn't met, instead just printing a warning?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sass/node-sass/pull/2440#issuecomment-406511594, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWP-u1P-0nvqGgHBMnViGrR-Oveodks5uIYSggaJpZM4VGwiO .
Node.js team has added support for N-API on Node 6 and Node 8, what difference does that make for users to use N-API or nan? If the concern is about users using older versions, they can keep using node-sass@4 (SEMVER major are about breaking changes after all), I don't really see the point of keeping nan when we could use N-API everywhere.
Picking this up again as we plan to drop support for Node < 10 in the next semver major.
