Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ecosystem Modules that will break in Node 12 #25060

Closed
targos opened this issue Dec 15, 2018 · 65 comments
Closed

Ecosystem Modules that will break in Node 12 #25060

targos opened this issue Dec 15, 2018 · 65 comments
Labels
meta Issues and PRs related to the general management of the project.

Comments

@targos
Copy link
Member

targos commented Dec 15, 2018

This is an issue meant to track modules that are currently skipped in citgm because they (or their tests) are currently broken on current versions of Node.

Fixed Module name Node version Reason Refs
uglify-js master Output expectation in tests mishoo/UglifyJS#3304
time 11, master Crash when called with NaN TooTallNate/node-time#76
stylus 11, master Fixed version unreleased stylus/stylus#2436
graceful-fs 11, master Tests broken isaacs/node-graceful-fs#137
bcrypt master Using removed V8 API kelektiv/node.bcrypt.js#684
microtime master Using removed V8 API wadey/node-microtime#61
serialport master Using removed V8 API serialport/node-serialport#1742
ref master Using removed V8 API TooTallNate/ref#107
node-report master Using removed V8 API nodejs/node-report#116
sqlite3 master Using removed V8 API TryGhost/node-sqlite3#1093
leveldown master Using removed V8 API Level/leveldown#540
level master Using removed V8 API Level/level#123
iconv master Using removed V8 API bnoordhuis/node-iconv#198
node-gyp master Using removed V8 API nodejs/node-gyp#1705

Feel free to edit this post to update the table.

@targos targos added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. meta Issues and PRs related to the general management of the project. labels Dec 15, 2018
@targos targos added this to the 12.0.0 milestone Dec 15, 2018
@richardlau
Copy link
Member

Duplicate of #25059. I like the table layout here but also prefer the title of #25059. We should probably pin whichever one we go with.

@targos
Copy link
Member Author

targos commented Dec 15, 2018

@mcollina Sorry I missed your issue. I also prefer to have a table layout. I added a column with checkboxes.

@mcollina
Copy link
Member

I’m ok of closing my issue but changing the title of this one.

@richardlau richardlau changed the title citgm: tracking skipped modules Ecosystem Modules that will break in Node 12 Dec 15, 2018
@richardlau richardlau pinned this issue Dec 15, 2018
@targos
Copy link
Member Author

targos commented Dec 15, 2018

I added sqlite3, leveldown, level and iconv to the list. Someone should open an issue for the first 3.

@mcollina
Copy link
Member

leveldown is fixed in the upcoming v5 release cc @ralphtheninja.

@silverwind
Copy link
Contributor

silverwind commented Dec 15, 2018

Filed mishoo/UglifyJS#3304 for uglify-js.

Not a big issue overall, they are basically testing the console output format, no real breakage, so I agree to ignoring it in citgm.

@reconbot
Copy link
Contributor

reconbot commented Dec 16, 2018

serialport should be fixed in master, lmk

@jdalton
Copy link
Member

jdalton commented Dec 16, 2018

@silverwind Might want to see if this affects terser.

@silverwind
Copy link
Contributor

@jdalton terser/terser#197.

@Flarna
Copy link
Member

Flarna commented Dec 17, 2018

Quite some of the fails are because of NAN which is not yet prepared for Node 12 but V8 API has changed since Node 10/11.
To my understanding a V8 update to 7.2 is pending (#24875) which may include further breaking changes. Is it needed to create a Nan version for such an in between version of node?

@targos
Copy link
Member Author

targos commented Dec 17, 2018

According to the Chromium release calendar, I think we will aim for V8 7.4 in Node 12, maybe with a forward-compat patch to have the API of V8 7.5.
Knowing that, it might indeed be too early to adapt NAN. The problem is that in the mean time, we have no way to know if changes in core are going to break native modules that depend on NAN.

@mcollina
Copy link
Member

As far as I know, V8 does not plan to remove any of the non-deprecated methods in non-major releases. Is this assumption correct? cc @nodejs/v8.

I think the plan should be to just remove all usage of currently deprecated methods in V8 in master.

@targos
Copy link
Member Author

targos commented Dec 17, 2018

@mcollina that's correct, but what they often do is deprecate in a version (e.g. 7.3) and remove in the next (7.4).

@Flarna
Copy link
Member

Flarna commented Dec 17, 2018

Knowing that, it might indeed be too early to adapt NAN. The problem is that in the mean time, we have no way to know if changes in core are going to break native modules that depend on NAN.

I created nodejs/nan#831 I think this is a better place to discuss this in detail.

@hashseed
Copy link
Member

V8 removes APIs by first marking them V8_DEPRECATED, and waits at least one version bump (e.g. 7.3 to 7.4) before actually removing it.

That said, we take precautions for the time around Node.js LTS releases to not unnecessarily break ABI stability.

@hashseed
Copy link
Member

I created a roadmap for V8 in Node.js 12 here.

@richardlau
Copy link
Member

I updated iconv and node-report as they both fail again due to the V8 update to 7.4.

The new errors for those modules are coming from nan.

PR fix for node-report: nodejs/node-report#125

@devsnek
Copy link
Member

devsnek commented Mar 29, 2019

Math.pow now works identical to the exponential operator.

we might also want to call this out in the change log

@richardlau
Copy link
Member

Fixed node-report in 2.2.5.

@richardlau
Copy link
Member

Added node-gyp: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1781/nodes=debian9-64/testReport/junit/(root)/citgm/node_gyp_v3_8_0/

PR fix: nodejs/node-gyp#1705

@kibertoad
Copy link
Contributor

Sqlite and node-gyp fixes also merged, no?

@refack
Copy link
Contributor

refack commented Apr 2, 2019

Sqlite and node-gyp fixes also merged, no?

Merged but not shipped yet.

@joshunger
Copy link

Should this be added here? facebook/create-react-app#6891

@tripower
Copy link

tripower commented May 8, 2019

node-java v0.11.0 seems also not to be working with node v12.0.0
https://github.com/joeferner/node-java

@rvagg
Copy link
Member

rvagg commented May 8, 2019

bignum isn't working with 12 and it's causing some angst. justmoon/node-bignum#120

It could really do with a conversion to node-addon-api but the distance between nan and addon-api for object wrapped addons with lots of constructor work is a bit too much of a stretch for me with the limited mental bandwidth I can afford for it.

I have publish rights if someone wants to do the work though!

@sam-github
Copy link
Contributor

I fixed the test failure with the bignum PR porting o 12.x, but don't have time ATM for a rewrite.

@splitice
Copy link

splitice commented Jun 5, 2019

weak-napi (alternative to weak) is also broken. I have attempted to fix it but it's segfaulting sometimes for a reason that's beyond me.

@targos
Copy link
Member Author

targos commented Jun 5, 2019

The weak-napi test suite passes for me (Node.js 12.4.0):

$ citgm weak-napi
info:    starting            | weak-napi           
info:    lookup              | weak-napi           
info:    lookup-notfound     | weak-napi           
info:    lookup-githead      | https://github.com/node-ffi-napi/weak-napi/archive/3efed548b5cd5ec2f3f82858eeae8d9bbd5d26e6.tar.gz
info:    weak-napi npm:      | Downloading project: https://github.com/node-ffi-napi/weak-napi/archive/3efed548b5cd5ec2f3f82858eeae8d9bbd5d26e6.tar.gz
info:    weak-napi npm:      | Project downloaded weak-napi-1.0.3.tgz
info:    weak-napi npm:      | npm install started 
warn:    weak-napi npm-install:| In file included from ../src/weakref.cc:2:                                                                                                                                                                                                         
warn:                          | /tmp/9300544d-093c-4fad-bf41-11189928c9ac/weak-napi/node_modules/setimmediate-napi/include/setimmediate.h: In instantiation of ‘void SetImmediate(napi_env, T&&) [with T = {anonymous}::ObjectInfo::OnFree()::<lambda()>; napi_env = napi_env__*]’:
warn:                          | ../src/weakref.cc:25:6:   required from here                                                                                                                                                                                                       
warn:                          | /tmp/9300544d-093c-4fad-bf41-11189928c9ac/weak-napi/node_modules/setimmediate-napi/include/setimmediate.h:46:7: warning: catching polymorphic type ‘class Napi::Error’ by value [-Wcatch-value=]                                                   
warn:                          | 46 |     } catch (Napi::Error e) {                                                                                                                                                                                                                 
warn:                          | |       ^~~~~                                                                                                                                                                                                                                      
info:    weak-napi npm:      | npm install successfully completed
info:    weak-napi npm:      | test suite started  
info:    passing module(s)   |                     
info:    module name:        | weak-napi           
info:    version:            | 1.0.3               
info:    done                | The smoke test has passed.
info:    duration            | test duration: 13746ms

Do you have a reproducible test case that segfaults?

@splitice
Copy link

splitice commented Jun 5, 2019

@targos Heres the corresponding issue node-ffi-napi/weak-napi#16

I've got to the point that the test cases pass (using nodejs/node-addon-api#475), but real world stressing code triggers segfaults. I'm on 12.3.x I'll upgrade my test to 12.4.0 and re-confirm with vanilla napi and patched.

@splitice
Copy link

splitice commented Jun 5, 2019

I can confirm the issue exists with 12.4.0 both with vanilla napi and patched.

@nickdesaulniers
Copy link

node-nanomsg broken in v12: nickdesaulniers/node-nanomsg#206

@ybiquitous
Copy link

Hi, graceful-fs seems to support Node 12 now. See Travis build below:

https://travis-ci.org/isaacs/node-graceful-fs/jobs/550978466

@johnnysprinkles
Copy link

Not sure if https://github.com/TooTallNate is active on the ref package -- and that's blocking all use of FFI on Node 12...

@silverwind
Copy link
Contributor

silverwind commented Jul 12, 2019

Do we need to keep this open? Node 12 is released and I understand this issue was to track issues before release. Posting Node 12 issues here will not magically fix them.

@ybiquitous
Copy link

ybiquitous commented Jul 12, 2019

The head version of stylus seems to accept Node 12. See the CI log below:
https://travis-ci.org/stylus/stylus/jobs/552691143#L439

But not released yet…

@targos
Copy link
Member Author

targos commented Jul 12, 2019

@silverwind You're right, let's close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests