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

Timeline to remove process.binding debt from core? #416

Closed
bmeck opened this issue Sep 6, 2019 · 6 comments
Closed

Timeline to remove process.binding debt from core? #416

bmeck opened this issue Sep 6, 2019 · 6 comments

Comments

@bmeck
Copy link
Member

bmeck commented Sep 6, 2019

There is an odd comment about readable-stream requiring a call to process.binding, but in a cursory glance on this repo I don't see usage that gets access to the native source text to evaluate.

https://github.com/nodejs/node/blob/63b056d8d4f0696254cd5fc40a69aee0157fc410/lib/stream.js#L57

Is there some timeline to remove this code / when does the support for ancient forms of readable-stream expire?

@addaleax
Copy link
Member

addaleax commented Sep 6, 2019

I’m not sure if that’s what you’re asking here, but the code you linked to has an automatic fallback for when process.binding() does not return a valid result. There is no need to remove it, imo.

We should probably undo nodejs/node@c97851dcd8c5e for that file (and the other stream files), though.

@bmeck
Copy link
Member Author

bmeck commented Sep 6, 2019

Why/how is that code being used that the binding call needs to remain / streams.js seems to be working even with other internal specific API usage so I'm questioning the actual benefits of the odd fallback

addaleax added a commit to addaleax/node that referenced this issue Sep 6, 2019
…util"

This partially reverts commit c97851d.

Streams code should ideally require on public APIs as much as possible,
because it is exported as readable-stream.

Refs: nodejs#26698
Refs: nodejs/readable-stream#416
@addaleax
Copy link
Member

addaleax commented Sep 6, 2019

I don’t really understand what you are saying, sorry. The code is there for the versions exported as readable-stream, and the process.binding call is only used for older versions of Node.js.

@bmeck
Copy link
Member Author

bmeck commented Sep 6, 2019

The code is there for the versions exported as readable-stream, and the process.binding call is only used for older versions of Node.js.

This doesn't make sense to me, where is it being used?

@bmeck
Copy link
Member Author

bmeck commented Sep 6, 2019

Or to put it another way, why is the binding important, but the other internal only things not? It seems v12 passes tests even with a variety of missing internals due to rewriting source, but it doesn't cover primordials and it still passes. There isn't a disclaimer or anything in that file about it being special and the list of internal APIs being used in it is increasing over time:

v8

  • internal/streams/legacy
  • internal/util/types w/ fallback via process.binding
  • internal/buffer w/ fallback on userland impl

v10 added

  • internal/streams/pipeline
  • internal/streams/end-of-stream

removed

  • internal/util/types

v12 added

  • primordials
  • internal/util/types again with fallback

@mcollina
Copy link
Member

mcollina commented Sep 8, 2019

I think we can safely remove that block from core, it's from the old days and for an old line of readable-stream.

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

No branches or pull requests

3 participants