Skip to content
This repository has been archived by the owner on Nov 2, 2020. It is now read-only.

Added Node 14 support (#41) #42

Merged
merged 7 commits into from
Jul 3, 2020
Merged

Added Node 14 support (#41) #42

merged 7 commits into from
Jul 3, 2020

Conversation

EricRabil
Copy link
Contributor

@EricRabil EricRabil commented Apr 29, 2020

I only have a Mac, and am unsure how to build for Linux and Windows. However the Darwin build for Node 14 is working for me, I'll test it out on my projects and see how it fares.

Update: running fine for me on 14.1.0, however npm run test within the cWS repo is failing with the following:

ericrabil@Erics-Kitchen cWS % npm run test

> @clusterws/cws@1.6.0 test /Users/ericrabil/eric.git/cWS
> nyc mocha './tests/**/*.test.ts' --exit

  CWS Server & Client Tests SSL
dyld: lazy symbol binding failed: Symbol not found: __ZN4node10BaseObject12FromJSObjectEN2v85LocalINS1_5ValueEEE
  Referenced from: /Users/ericrabil/eric.git/cWS/dist/bindings/cws_darwin_83.node
  Expected in: flat namespace

dyld: Symbol not found: __ZN4node10BaseObject12FromJSObjectEN2v85LocalINS1_5ValueEEE
  Referenced from: /Users/ericrabil/eric.git/cWS/dist/bindings/cws_darwin_83.node
  Expected in: flat namespace

@codecov-io
Copy link

codecov-io commented Apr 30, 2020

Codecov Report

Merging #42 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #42   +/-   ##
=======================================
  Coverage   65.68%   65.68%           
=======================================
  Files           4        4           
  Lines         204      204           
  Branches       57       57           
=======================================
  Hits          134      134           
  Misses         70       70           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b95c0f...3816c30. Read the comment docs.

@EricRabil
Copy link
Contributor Author

EricRabil commented Apr 30, 2020

Just missing Windows bindings, adding 14 support was pretty easy I just need to compile them on the various platforms. I deployed cWS for Node 14 to my prod servers and it's running fine.

@goriunov
Copy link
Member

goriunov commented May 9, 2020

@EricRabil Thank you for your PR. Please rebase this branch on latest master also master includes fix for Symbol not found for Node 13 you may want to add the same for Node 14:

// Node 13 fix In Addon.h
#if NODE_MAJOR_VERSION==13
  #include "headers/13/tls_wrap.h"
  #include "headers/13/base_object-inl.h" // Need to add this line
#endif

Copy link
Member

@goriunov goriunov left a comment

Choose a reason for hiding this comment

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

Looks pretty good ! Just one minor change needed

make.bat Outdated
)

cl /I targets/node-%v64%/include/node /I targets/node-%v64%/deps/uv/include /I targets/node-%v64%/deps/v8/include /I targets/node-%v64%/deps/openssl/openssl/include /I targets/node-%v64%/deps/zlib /EHsc /Ox /LD /Fedist/bindings/cws_win32_64.node src/*.cpp targets/node-%v64%/node.lib
cl /I targets/node-%v72%/include/node /I targets/node-%v72%/deps/uv/include /I targets/node-%v72%/deps/v8/include /I targets/node-%v72%/deps/openssl/openssl/include /I targets/node-%v72%/deps/zlib /EHsc /Ox /LD /Fedist/bindings/cws_win32_72.node src/*.cpp targets/node-%v72%/node.lib
cl /I targets/node-%v79%/include/node /I targets/node-%v79%/deps/uv/include /I targets/node-%v79%/deps/v8/include /I targets/node-%v79%/deps/openssl/openssl/include /I targets/node-%v79%/deps/zlib /EHsc /Ox /LD /Fedist/bindings/cws_win32_79.node src/*.cpp targets/node-%v79%/node.lib
cl /I targets/node-%v83%/include/node /I targets/node-%v83%/deps/uv/include /I targets/node-%v83%/deps/v8/include /I targets/node-%v83%/deps/openssl/openssl/include /I targets/node-%v83%/deps/zlib /EHsc /Ox /LD /Fedist/bindings/cws_win32_79.node src/*.cpp targets/node-%v83%/node.lib
Copy link
Member

Choose a reason for hiding this comment

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

/Fedist/bindings/cws_win32_79.node should point to 83 -> /Fedist/bindings/cws_win32_83.node

@goriunov
Copy link
Member

goriunov commented May 10, 2020

Also this branch should not include any bindings for other versions except node 14

@EricRabil
Copy link
Contributor Author

Could you provide a bit more insight as to why you'd like bindings for versions earlier than 14 to be removed?

Node 12 is the latest LTS, we should at least include support for that.

@EricRabil EricRabil requested a review from goriunov June 20, 2020 06:33
@goriunov
Copy link
Member

@EricRabil will you are rebuilding already existing bindings as they already exists in master branch there is no need to change anything in those. So after merging this branch it should just merge new bindings only for version 14. TBH i would prefer to rebuild all bindings including version 14 before releasing to npm to be on the safe side.

@EricRabil
Copy link
Contributor Author

I removed the earlier bindings. I wasn’t able to build a windows binding for 14, but that’s because I couldn’t get a Windows development environment set up.

@goriunov
Copy link
Member

@EricRabil better not to delete those bindings but remove changes which were done (you should be able to just copy already existing one from master branch) :)

@EricRabil
Copy link
Contributor Author

EricRabil commented Jun 21, 2020

Ahh sorry, I’ll fix that.

Edit: the original bindings are now in this branch.

@EricRabil
Copy link
Contributor Author

@goriunov hi there, are there any other changes you need from me? :)

@goriunov
Copy link
Member

goriunov commented Jul 3, 2020

Looks good, i will try to merge this PR soon ! Thank you !

@goriunov goriunov merged commit 707a91a into ClusterWS:master Jul 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants