node: deprecate public access to process.binding
#2768
Conversation
vkurchatkin
added
the
semver-major
|
Can you change |
| var assert = require('assert'); | ||
|
|
||
| assert.throws( | ||
| function() { | ||
| process.binding('test'); |
There was a problem hiding this comment.
Perhaps leave the process.bindings in this test so that the deprecated functionality is still tested before it is removed?
|
+1. Can the reason that this is semver-major be changed to keep it semver-minor? |
|
No, this should be semver-major. Landing this in 4.0.0 will not be good imo. We need a docs deprecation first if there isn't one already. That can land in 4.0.0, maybe. cc @nodejs/tsc this is a quite significantly large change. |
|
be very careful, npm deps have usage of |
|
@bmeck There are a lot more. |
|
|
+1 for deprecating, because it was never actually supported: incompatible changes on the c++ side can be introduced in minor versions given that the js side compensates that. That results in problems if whoever is using those bindings directly. Partial usage (approx 1/3): This is a bit outdated. I will post an updated list if needed. |
@bmeck please get people to update graceful-fs. It won't work soon without updating anyways. |
|
I know the HTTP parser (access) was undocumented before, but there are modules that use it and having it publicly available in some way would be beneficial IMHO. |
|
+1 to change process.bindings to require('bindings/foo') but I think we should not remove process.bindings, use |
|
@thefourtheye I didn't really add new declarations @mscdex I agree, but that is another issue, we should expose http parser via safe and stable API |
|
If possible I'd like to land this behind a flag for v5.0.0 so we can urge folks to run with that flag enabled to see if their project breaks. If we get significant pushback on it, we could revert the change, otherwise we'd make it the default in v6.0.0. |
|
-1 on just doing this in a semver-major, +1 for using |
|
I agree with @rvagg; the npm team has mostly landed the changes necessary for this to not be a problem for npm, but I think npm is a bellwether here. Deprecating first is a pain for the Node team, but not deprecating it is a major pain for users. In particular, it's going to take a while for people to stop using versions of To be clear, npm/npm@2b97a57 removes the only direct use of Once that work is done, which should happen as of the next versions of |
|
Isn't there some sort of precedence we have for commonly used "internals"? Like one such underscored header on http? Also, we have received valid bug reports from users tapping into the direct bindings. It's been helpful to get user feedback using those functions that I highly doubt would have happened if it were behind a flag. -1 from me unless someone can point me to issues having it exposed has caused. |
|
@trevnorris |
|
@vkurchatkin IIRC graceful-fs was doing worse than tapping into a binding. To add, it's still easy to bypass a chunk of what's accessible via bindings by abusing things like |
|
|
Updated list, using |
|
LGTM once the merge conflicts get sorted out. @trevnorris I want to bake the bindings into the snapshot (eventually) but that won't work as long as Apart from that, we had to revert an improvement in lib/fs.js a while ago because of graceful-fs, ffs. That alone is a good reason to deprecate and remove it. |
|
@bnoordhuis I'm not sure if there was any resolution, but I'm still eager to pursue this. People abuse this API without even thinking or giving any warning at least. A fresh example: https://github.com/mafintosh/why-is-node-running. |
|
@vkurchatkin This looks like a good change to me, but afaik it's blocked by #3307 at least, and perhaps some other valid use-cases. |
|
If I recall correctly the last time the CTC discussed this it came down to the potential ecosystem breakage being too significant to do this any time soon. It may be worth looking at again. If we can come up with a good transition path then maybe it's something we can do in an upcoming major post v6 |
|
Here is the revised plan:
|
|
Returning to this... That approach SGTM @vkurchatkin |
|
-1 from me, it is sometimes the only way to write useful things (for example, in node-spdy) |
|
Perhaps that just means we need to do a better job at exposing a better
|
|
@jasnell I agree. However this task is much more complicated than what is suggested here, and will take quite a lot of time. Thus, -1 from me. |
Can you explain, why? I seriously doubt that, but if it's the case, than we have a serious gap in API. It might be a shortcut for someone who know internals to make things faster, but the downside is that
Any suggestions are welcome. Time is not the issue, but I believe it's important to move in this direction. Step 1 is pretty harmless, for example |
|
node-spdy provides compatibility with core's |
|
@indutny I suppose you are talking about this: https://github.com/indutny/http-deceiver/blob/master/lib/deceiver.js It's exactly the kind of thing I'm trying to prevent. |
estliberitas
force-pushed
the
master
branch
2 times, most recently
from
7da4fd4
to
c7066fb
Compare
|
@vkurchatkin And I think @indutny's saying that's exactly what he needs. There's no reason to disallow access to everything in And preventing the user from accessing these in many cases is futile. e.g. net.createServer().listen(8081)._handle.constructor === process.binding('tcp_wrap').TCPIn these cases it especially doesn't make sense to remove access to the constructor via a simple API call. |
|
Given the -1's and the lack of further discussion on this, closing. |
Perhaps we should stick to the proposal in #2768 (comment), but only the two first steps for now and on case-by-case basis? I.e. do this for some cases that shouldn't be allowed, if I understood things correctly, but allow (keep accessible via |
|
We could start with hiding those ones that have a clear replacement, e.g. |
|
@ChALkeR Sounds like a reasonable place to start. |
| @@ -1,4 +1,5 @@ | |||
| 'use strict'; | |||
| // Flags: --expose_internals | |||
There was a problem hiding this comment.
Only if process.binding is actually deprecated
There was a problem hiding this comment.
No, where is require('binding/… used in this test? Note that it doesn't seem to call hasMultiLocalhost.
There was a problem hiding this comment.
No idea, it was too long ago. Don't bother reviewing, I'm gonna redo this from scratch
There was a problem hiding this comment.
As an idea: we can make a separate folder for tests that require expose_internals and set it implicitly.
Also, Before 6.3.0, There is nothing valueable in |
|
net.js:10 Error: EFILE Please Help, why am I getting this error and what is the way out ... |
This PR introduces a new way to access bindings:
require('binding/module_name')and deprecatesprocess.binding. This is a logical continuation of internal modules feature. Bindings are also accessible only in builtin modules or with--expose_internalsflag.R=@nodejs/collaborators