graceful-fs is likely the most-common but definitely not the only thing:

http://searchnod.es/#!binding(%22natives%22)/1

This kind of kicks the can down the road with process.binding, right? Is there any feasible way we can do this with internal modules?

I have an idea how we can deprecate public access to process.binding

For background, the reason for this PR is that I want to pre-compile our JS code using the V8 extras infrastructure. At that point process.binding('natives') can be removed if it weren't for modules like graceful-fs.

I think I'm ok with this. Making guarantees about our source being exposed at run-time is a ludicrous idea.

LGTM.

Thanks @jasnell. I plan to merge this early next week so if anyone has strong feelings, now is the time to speak up.

I have strong feelings about this being a good idea. 👍

@bnoordhuis I propose to use #2768 for this. It's not necessary to deprecate whole process.binding right away, but we can use introduced hooks to deprecate natives in javascript. Also someone could just replace natives with $natives to suppress warning. This is not possible with #2768

Fine by me, as long as it gets done. Closing.

@bnoordhuis It looks like it was actually never done, and #2768 was closed a few days ago.

Perhaps we should reopen this?

If this goes forward it should be updated to make use of the emitWarning() API.

Rebased. Added a second commit that takes a different approach: check if the topmost JS frame is a builtin script.

If this goes forward it should be updated to make use of the emitWarning() API.

Why? We don't do that for other warnings in C++ code either.

Why? We don't do that for other warnings in C++ code either.

So that it can be consistently handled as a runtime deprecation just like any other deprecation warning we print. As illustrated by #8270, it's pretty trivial and has the built-in benefit of working with --throw-deprecation.

Update ... for instance, with the patch as it is currently:

$ ./node --throw-deprecation
> const n = process.binding('natives')
(node:39039) process.binding('natives') is deprecated.
(node:39039) If you use graceful-fs < 4, please update.
(node:39039)    at repl:1
(node:39039)    at vm.js:22
(node:39039)    at vm.js:96
(node:39039)    at vm.js:21
(node:39039)    at repl.js:313
(node:39039)    at domain.js:280
(node:39039)    at domain.js:293
(node:39039)    at repl.js:504
(node:39039)    at events.js:101
(node:39039)    at events.js:188
(node:39039)    at readline.js:232
(node:39039)    at readline.js:572
undefined
>

Using emitWarning(), the warning would obey the --trace-deprecation and --throw-deprecation command line flags without any additional effort, and this PR could be simplified (without the extraneous (node:pid} outputs as well.

I found 268 lines in 147 packages using process.binding('natives'): https://gist.github.com/ChALkeR/d8c9ff68869a1d86a3d702ce5c6d3a0a.

• 103 lines of that are require(process.binding('natives').util ? 'util' : 'sys') with syntaxical differences.
• 32 lines of the remaining are inside tests.
• Without that — there are 133 files in 114 packages.

Is this intentional?

Yes. Please be specific if you think there is something unintentional in there.

@bnoordhuis Ok, I meant that the warning looks like too heavy to me, and most of those is cosumed by lines that do not bring much value (i.e. at module.js:…).

Btw, one more thing: 4 packages actually assign to process.binding('natives')[something], and they succeed in that. That happens inside tests only, though.

Could we stop that, like now, without waiting for a whole major release cycle? This shouldn't get misused even more.

Could we stop that, like now, without waiting for a whole major release cycle?

That would be nice, making props non-writable and non-configurable should be enough

Could we stop that

+1 ... especially if we know we can do it without breaking non-test code.

I meant that the warning looks like too heavy to me, and most of those is cosumed by lines that do not bring much value (i.e. at module.js:…).

I added the stack trace based on prior experience that if you don't, people are going to complain it's impossible to find out what is causing the warning.

I added the stack trace based on prior experience that if you don't, people are going to complain it's impossible to find out what is causing the warning.

Makes sense. But could we strip out lines that match \s(internal/)?module\.js:? Those are pointing to internal files, if I understand things correctly.

Added a commit that does that (but in a better way than pattern matching strings.)

@bnoordhuis Thanks!

Upd: yes, the warning looks good now.

@bnoordhuis Sorry, I have another warning message stylistic question. How much extra code would it be to print the If you use graceful-fs < 4, please update. line only for those cases where graceful-fs is present in the backtrace? That would remove possible confusion for those users that get this warning for reasons other than graceful-fs.

Upd: e.g. webpack does that on it's own, without graceful-fs. I can't guarantee that that exact code is commonly triggered, but webpack has almost 2 millon downloads per month. It's not an argument against this PR (the fix for webpack is straightforward), it's an argument for not confusing those users with graceful-fs specific mesages.

Btw — is this semver-patch or semver-major?

I'm -1 on this as is given that it is printed as a deprecation but does not properly follow the --trace-deprecation and --throw-deprecation command line flags. The behavior is inconsistent when it simply needn't be.

@jasnell, would you change your mind if this would follow the command-line flags?

Yep... see #2741 (comment)

However, please keep in mind that the new fix for graceful-fs uses the natives module which uses process.binding('natives'), which is what has allowed us to get past the fs re-evaluation issue.

@jasnell, older graceful-fs@3 versions used process.binding('natives') likewise, nothing has changed on that part. Btw, graceful-fs@4 should be fine, and most users have already upgraded to that.

Yep, just wanted to make sure it was noted.

I'm -1 on this as is given that it is printed as a deprecation but does not properly follow the --trace-deprecation and --throw-deprecation command line flags. The behavior is inconsistent when it simply needn't be.

The JS API has no way to do the internal stack frame filtering that @ChALkeR requested, and besides, other deprecation warnings from C++ (e.g. deprecated Template::Set usage) don't use it either.

I could argue the PR in its current shape is more consistent with existing practices than it would be if I included your suggestion.

At the very least --throw-deprecation and --trace-deprecation should be supported here in order for it to be a proper runtime deprecation... regardless of whether it uses emitWarning().

@bnoordhuis

The JS API has no way to do the internal stack frame filtering that @ChALkeR requested

Just a note: I would be fine with not filtering if printing the trace would be behind a flag. I don't have any specific opinion on printing from js vs printing from the C++ side here at the moment, though.