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

Node internals usage #333

Open
addaleax opened this issue Jan 29, 2018 · 4 comments
Open

Node internals usage #333

addaleax opened this issue Jan 29, 2018 · 4 comments

Comments

@addaleax
Copy link

addaleax commented Jan 29, 2018

Hi everyone!

As you probably know (because you are maintaining this code) spdy currently makes heavy use of assumptions about Node's internals, and in particular I'm thinking about the whole handle-thing stuff.

Obviously, breaking ecosystem modules like spdy is not a goal for Node, but this still puts Node core maintainers in a situation where it's unreasonable to take special care about not breaking spdy when the rest of the ecosystem, who plays by the rules and relies on public APIs instead, would benefit from changes.

So:

  • In the short term, how sure it is that you are willing to keep up with changes happening in Node yourself? I am opening this issue because I'm working on some stuff that does seem to break spdy; Those would go into a semver-major release on Node's side anyway, and would probably be easy to fix, but who's to say that that's always going to be the case?
  • In the long term, what do you need from Node core to avoid this problem altogether? It seems you're using code to create custom net.Socket instances, but it's not really clear to me why that requires hooking into Node's internals?
@jgautier
Copy link

@addaleax I am interested in getting this module to work in node v10. I am curious if you have any idea what the breaking were changes were in node core?

@diasdavid Has there any been any work already on making this work with v10?

@addaleax
Copy link
Author

@jgautier I wasn’t even aware that this is broken on Node 10 (and I’m not sure it was when I originally posted this).

Looking it up, we removed it from our internal test matrix in nodejs/citgm#564:

SPDY relies on spdy-transport and that broke due to a BC in 10.0.0.
However, neither of these modules are maintained anymore as they
got pretty much obsolete due to http2.

I would agree with that assessment, it’s a good idea to migrate away from it altogether.

shesek added a commit to shesek/spark-wallet that referenced this issue Sep 1, 2018
This works, but it looks like the "spdy" module (which, despite its name, also supports http2)
is not compatible with newer nodejs versions and should be avoided:
spdy-http2/node-spdy#333
webpack/webpack-dev-server#1451

Should eventually use nodejs's native http2 module, but it is currently incompatible with express:
expressjs/express#3388

Committing the changes to a branch and setting aside for now.
@vytautas-pranskunas-
Copy link

vytautas-pranskunas- commented Sep 20, 2018

So what are the recommendations?
Any ideas on how to fix it - nodejs 10+ to get working with spdy?

@coolaj86
Copy link

coolaj86 commented Oct 9, 2018

@vytautas-pranskunas- Check the comments at the bottom of #343

The problem, as I understand it, is that fixing v10 (current) breaks v8 (stable).

Perhaps someone (#339) could publish a new version with the updates and make good use of the engines field in package.json https://docs.npmjs.com/files/package.json#engines

As to the topic at hand... I have no idea how anyone can do any degree of network protocol work in node without relying on internal APIs... it's a jungle of very difficult to navigate code and not much is exposed to the outside world (though it's getting better all the time).

I myself have code that will only run on early versions of v8, none of v9, and only early versions of v10 because I'm operating in-between the tls and net layers, which were not build to be used independently. I imagine spdy suffers from similar issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants