Refactoring to use ws napi modules #302
Conversation
andreek
changed the title
andreek
force-pushed
the
master
branch
2 times, most recently
from
275f888 to
9f5dda7
Compare
|
@ibc - What do you think about going with this approach instead of continuing to include and maintain native modules directly? |
|
Definitely it looks good, less to maintain. |
- removed native modules - change imports - add napi dependencies for native modules
|
Hey @theturtle32 are you interested in external contributors's help on making a N-API version of this package? |
|
Definitely
…On Mon, Jun 29, 2020 at 2:36 PM Patricio Palladino ***@***.***> wrote:
Hey @theturtle32 <https://github.com/theturtle32> are you interested in
external contributors's help on making a N-API version of this package?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#302 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAM7QEGJ5MDP7LG44QN4F3RZECMFANCNFSM4EGP3DWA>
.
|
|
That's great! Me or @nebojsa94 will take a look at this and help to migrate this to n-api |
|
Also take this into account: #302 |
|
@nebojsa94 or @alcuadrado - Is this existing pull request viable? Could it be used as a starting point? |
|
@theturtle32 I'm on it right now. It looks viable at first look. Have the native modules in here been modified? Or are they just vendored versions of |
|
I was never confident if it's ok to remove the fallbacks. I assumed they're used if build of native modules goes wrong. @alcuadrado as far as I remember the native modules were not available as separated packages from |
IMO N-API based modules always need fallbacks, as people may run them on operating systems or architectures that don't have prebuilt binaries. For example, most don't have ARM binaries. Both
I checked this and both the local and the npm versions of these packages expose the same functions, so they should be a linear replacement. @theturtle32 I think this PR is all that's needed to move this package to N-API. The only thing that may need some consideration is if the change in Note that GitHub shows @andreek's |
|
This looks good, @theturtle32 Do you think that changes in |
|
Hey @theturtle32, sorry to bother you again. Is there anything we can do to help move this forward? This is change is pretty important to us, as we are currently working with multiple projects to move the node-based Ethereum ecosystem into N-API based dependencies, and one of our core libraries depends on One of the main reasons we are doing this is that non N-API native libraries require a somewhat particular setup, especially on windows. This setup can be tricky, slow, and normally requires downloading gigabytes of data, making it a blocker for many new developers. This is a recurrent issue in workshops and other educational contexts, where time and bandwidth constraints make setting up the environment almost impossible. While we are moving forward with this effort in other libraries, and are close to moving most to N-API, a single one requiring this setup can spoil our efforts. We could move from Thanks! |
Considering the fallbacks in |
|
Hey, sorry for the delay. I was on vacation. I'll try to take a look this
week.
Brian
…On Mon, Jul 6, 2020 at 2:40 PM Patricio Palladino ***@***.***> wrote:
Hey @theturtle32 <https://github.com/theturtle32>, sorry to bother you
again. Is there anything we can do to help move this forward?
This is change is pretty important to us, as we are currently working with
multiple projects to move the node-based Ethereum ecosystem into N-API
based dependencies, and one of our core libraries depends on websocket.
One of the main reasons we are doing this is that non N-API native
libraries require a somewhat particular setup, especially on windows. This
setup can be tricky, slow, and normally requires downloading gigabytes of
data, making it a blocker for many new developers. This is a recurrent
issue in workshops and other educational contexts, where time and bandwidth
constraints make setting up the environment almost impossible.
While we are moving forward with this effort in other libraries, and are
close to moving most to N-API, a single one requiring this setup can spoil
our efforts. We could move from websocket into another library if you
don't agree with using the N-API libraries, but that doesn't seem to be the
case.
Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#302 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAM7QD7PYZC3EY2SCXFP33R2JAFZANCNFSM4EGP3DWA>
.
|
|
Thanks for the reply, Brian. Hope you had a great time. I tried running the tests in node 0.10.x, to understand if a major version bump was needed, but I failed to install the project. I got an HTTPS error that I tried to workaround, but I couldn't. |
|
Hey @theturtle32, any updates on this? |
|
@alcuadrado Looks good. Will merge. |
|
Well, I'll test first, and then merge. |
|
Hey @theturtle32, hope you're doing well! I've forked @andreek fork and added GitHub actions workflow for testing, you can check out the results here: https://github.com/Tenderly/WebSocket-Node/pull/1/checks?check_run_id=958210778. Is there anything else on our side we can do to help you get this upstream? |
|
Incorporated into #394 |
|
@andreek @ibc @nebojsa94 - This has been published on npm as v1.0.32 |
|
@theturtle32 do you mind tagging the commit on github :) |
Done. |
|
Thanks so much ! 👍 |
Should probably be merged into a n-api branch.
#284