Convert binding to N-API #2440

xzyfer · 2018-07-08T15:07:48Z

Heavy work in progress of replacing NAN with N-API.

Attempting to combine the prior work boingoing#1 with the drift in master, namely #2128 #2298.

Fixes #1988

xzyfer · 2018-07-11T15:03:09Z

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.

xzyfer · 2018-07-11T15:05:48Z

This potentially unwinds @kkoopa's work to propagate async context (#2295) because I couldn't find the equivalent in N-API.

xzyfer · 2018-07-12T14:09:14Z

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...

xzyfer · 2018-07-16T14:10:12Z

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.

xzyfer · 2018-07-16T15:29:29Z

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.

xzyfer · 2018-07-16T15:45:48Z

@nschonni note this means we probably can't have node-gyp fallback to build

nschonni · 2018-07-16T15:57:04Z

No, the node-pre-gyp handles napi vs the module version in it's download and fallback

xzyfer · 2018-07-16T16:00:24Z

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.

nschonni · 2018-07-16T16:14:30Z

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 💯

xzyfer · 2018-07-16T16:29:18Z

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, ***@***.***> 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 <#2440 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWEVhL0lhyjPYXpGzE9DtAqzfK9nUks5uHLvogaJpZM4VGwiO> .

This was a non-standared feature in LibSass we had to support. It is being removed in LibSass 3.6.0. This prevents people from output their source directory. See sass/libsass#2611 See #2184 See #2006 See #1933 See #1925 See #1867 See #1845

Four years ago there was a test that mutated `process.cwd` but didn't correctly reset it. The altered global state resulted in future tests having incorrect assetions. I only noticed this because for some reason it manifested in failures in only _some_ Node versions when N-API was being used.

Fixes #2487

This flag means we skip a bunch of code paths in CI which is not ideal. Since we tell people to use `npm rebuild` we should eat our own dog food. With this patch we do an install, then do a rebuild hitting all the major install code paths. Fixes #1453

Followup-to: #2553

saper · 2019-11-08T23:39:47Z

src/binding.cpp

+    indent_type_len == 1 ? '\t' : ' '
+  ).c_str());
+
+ napi_value propertyLinefeed;


indentation problem?

saper · 2019-11-08T23:49:18Z

binding.gyp

      'xcode_settings': {
        'CLANG_CXX_LANGUAGE_STANDARD': 'c++11',
+        'OTHER_CPLUSPLUSFLAGS': [
+          '-std=c++11'
+        ],


why do need this? should be covered by line 33 already...

saper · 2019-11-08T23:50:59Z

lib/extensions.js

+      // We can opt-in to later versions as we choose to adopt new API features.
+      //
+      // See https://nodejs.org/api/n-api.html#n_api_n_api_version_matrix
+      Math.min(process.versions.napi, 3),


Will napi version change when libuv interface is modified? we use that heavily as well.

saper · 2019-11-08T23:59:51Z

src/sass_types/color.cpp

+  napi_value Color::getConstructor(napi_env env, napi_callback cb) {
+    napi_value ctor;
+    napi_property_descriptor descriptors[] = {
+      { "getR", nullptr, GetR },


../src/sass_types/color.cpp:66:31: warning: missing field 'getter' initializer [-Wmissing-field-initializers] { "getR", nullptr, GetR },

and for others, too

saper · 2019-11-09T00:37:59Z

src/callback_bridge.h

@@ -2,30 +2,32 @@
 #define CALLBACK_BRIDGE_H

 #include <vector>
-#include <nan.h>
 #include <algorithm>
 #include <uv.h>


Including <uv.h> breaks guarantees of ABI stability offered by N-API. #2312 (comment)

saper · 2019-11-09T00:57:57Z

src/sass_types/value.h


 namespace SassTypes
 {
  // This is the interface that all sass values must comply with
-  class Value : public Nan::ObjectWrap {


We probable need to rebuild this without inheritance at all. Maybe this should become a template? We definitely need https://github.com/nodejs/node-addon-api/blob/master/doc/object_wrap.md here

…2620) * fix: the function only need to execute once enough * fix: typo `;`

gabrielschulhof · 2020-07-06T15:27:03Z

Is anyone still working on this 👋

…overread Fix file content malloc to avoid reading beyond buffer

xzyfer self-assigned this Jul 8, 2018

xzyfer added Dev - WIP Module - Binding labels Jul 8, 2018

xzyfer force-pushed the napi branch from 75b9c9e to 8690e6c Compare July 11, 2018 15:00

xzyfer mentioned this pull request Jul 11, 2018

Look at N-API to see if it would help our binding portablitiy #1988

Open

xzyfer force-pushed the napi branch 4 times, most recently from a342097 to cec10ba Compare July 12, 2018 14:00

xzyfer force-pushed the napi branch 2 times, most recently from 96b5e31 to 73a9cea Compare July 16, 2018 14:08

xzyfer changed the base branch from master to v5 July 16, 2018 14:09

xzyfer changed the title ~~[WIP] N-API~~ Convert binding to N-API Jul 16, 2018

xzyfer force-pushed the v5 branch from 26b99a0 to 6d6e407 Compare July 16, 2018 14:44

xzyfer force-pushed the napi branch 5 times, most recently from a9ba52c to fa03e0f Compare July 16, 2018 15:09

xzyfer force-pushed the napi branch from fa03e0f to a7116b4 Compare July 16, 2018 15:41

xzyfer and others added 19 commits November 8, 2019 22:21

Bump mocha@^6.2.2

0b580ad

Bump fs-extra@^8.1.0

9685515

Bump eslint@^6.6.0

e717d02

Bump node-gyp@^6.0.1

6cb1245

Remove deprecated process.sass API

0fd8072

Remove workarounds for old Node.js versions

40303f0

Replace lodash/assign in favor of the native Object.assign

80fef6f

Indicate the minimum supported Node.js version in package.json

24dc20d

Compile on watch

625329e

Don't watch or compile .css files

c5edd85

This was a non-standared feature in LibSass we had to support. It is being removed in LibSass 3.6.0. This prevents people from output their source directory. See sass/libsass#2611 See #2184 See #2006 See #1933 See #1925 See #1867 See #1845

Bump coveralls@^3.0.7

e9c482f

Bump glob@^7.1.6

3b80208

Bump nan@^2.14.0

f933316

Bump stdout-stream@^1.4.1

c5b5b92

Bump true-case-path@^2.2.1

9fc32e4

Remove async-foreach dependency

e1735cb

Fixes #2487

Remove skip download for CI flag

d042f7b

This flag means we skip a bunch of code paths in CI which is not ideal. Since we tell people to use `npm rebuild` we should eat our own dog food. With this patch we do an install, then do a rebuild hitting all the major install code paths. Fixes #1453

Suppress -std=c++0x option

ac145b1

Followup-to: #2553

saper reviewed Nov 8, 2019

View reviewed changes

src/binding.cpp

indent_type_len == 1 ? '\t' : ' '

).c_str());

napi_value propertyLinefeed;

Copy link

Member

saper Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation problem?

saper reviewed Nov 9, 2019

View reviewed changes

givingwu and others added 2 commits November 9, 2019 18:13

fix: the getBinaryPath function only need to execute once enough (#…

7293699

…2620) * fix: the function only need to execute once enough * fix: typo `;`

Convert binding to N-API

23b571e

xzyfer force-pushed the napi branch from 439d98a to 23b571e Compare November 12, 2019 11:53

xzyfer force-pushed the v5 branch from a85ee70 to 895acbd Compare March 21, 2020 03:49

nschonni mentioned this pull request Sep 30, 2020

Switch binary build/cache to prebuild #2987

Open

jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024

Merge pull request sass#2440 from mgreter/bugfix/file-content-buffer-…

4869c0f

…overread Fix file content malloc to avoid reading beyond buffer

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert binding to N-API #2440

Convert binding to N-API #2440

xzyfer commented Jul 8, 2018 •

edited

Loading

xzyfer commented Jul 11, 2018 •

edited

Loading

xzyfer commented Jul 11, 2018

xzyfer commented Jul 12, 2018

xzyfer commented Jul 16, 2018

xzyfer commented Jul 16, 2018

xzyfer commented Jul 16, 2018

nschonni commented Jul 16, 2018

xzyfer commented Jul 16, 2018

nschonni commented Jul 16, 2018

xzyfer commented Jul 16, 2018 via email

saper Nov 8, 2019

saper Nov 8, 2019

saper Nov 8, 2019

saper Nov 8, 2019

saper Nov 9, 2019

saper Nov 9, 2019

gabrielschulhof commented Jul 6, 2020

Convert binding to N-API #2440

Are you sure you want to change the base?

Convert binding to N-API #2440

Conversation

xzyfer commented Jul 8, 2018 • edited Loading

xzyfer commented Jul 11, 2018 • edited Loading

xzyfer commented Jul 11, 2018

xzyfer commented Jul 12, 2018

xzyfer commented Jul 16, 2018

xzyfer commented Jul 16, 2018

xzyfer commented Jul 16, 2018

nschonni commented Jul 16, 2018

xzyfer commented Jul 16, 2018

nschonni commented Jul 16, 2018

xzyfer commented Jul 16, 2018 via email

saper Nov 8, 2019

Choose a reason for hiding this comment

saper Nov 8, 2019

Choose a reason for hiding this comment

saper Nov 8, 2019

Choose a reason for hiding this comment

saper Nov 8, 2019

Choose a reason for hiding this comment

saper Nov 9, 2019

Choose a reason for hiding this comment

saper Nov 9, 2019

Choose a reason for hiding this comment

gabrielschulhof commented Jul 6, 2020

xzyfer commented Jul 8, 2018 •

edited

Loading

xzyfer commented Jul 11, 2018 •

edited

Loading