@ljharb Thank you for submitting this PR!

🔔 @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @Flarna @mwiktorczyk @wwwy3y3 @DeividasBakanas @kjin @alvis @eps1lon @Hannes-Magnusson-CK @jkomyno @ajafff @hoo29 @n-e @BrunoScheufler @mohsen1 @KSXGitHub @a-tarasyuk @islishude @r3nya @eyqs @ZaneHannanAU @ThomasdenH @matthieusieben - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@ljharb The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Looks like the node types prior to v9 lack types for the NodeBuffer constructor itself - I'll revert those, and only include the v9+ type fix.

I'm not sure what this change is needed for. What's wrong with the new APIs defined below declare var Buffer? Could you add a test to show the use case enabled by this?
Why only NodeJS 9 and not latest nodejs?

@Flarna i have a place where i do obj.constructor.isBuffer, and this was erroring without this change. I only included the fix for v9 because the older versions don’t seem to have types for the constructor (#28900 (comment))

I’ll try to add a test.

@ljharb With latest NodeJs I mean the file types\node\index.d.ts.
I wonder a little about your usecase. Why not using the static API Buffer.isBuffer()?
If Buffer definition would be changed to class with static methods your usage wont work also as far as I know as typeof <some Buffer instance>.constructor would be of type Function.

Because if i do that, bundlers include an entire Buffer polyfill - see ljharb/qs#39.

As for the type, for any builtin T besides Function, typeof new T().constructor should always be T - if it’s only Function, then that’s a bug in the type system.

ok understood.

As for the type, for any builtin T besides Function, typeof new T().constructor should always be T - if it’s only Function, then that’s a bug in the type system.

Seems so yes, see microsoft/TypeScript#3841

In that case, do you suggest a different approach? This is a pretty basic part of JS to have been mistyped for this long :-/

I don't have a different approach.
At first I haven't understood why this is needed at all, then I thought about converting Buffer to class - assuming that this solves your problem. But it turned out it doesn't and googling routed me to above typescript issue. I haven't read all comments there, not sure if there is something better we could use here.

@Flarna thanks, updated (i missed it because i assumed there'd be a v10 folder)

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@ljharb Did you still want to add a test to this?

Why we have EventEmitter as a class but not Buffer?

TypeScript enforces new for classes. Note that real classes are not callable without new in runtime.

But Node "class"es are not real classes and are just plain functions.

We have to decide if we want to use class for Node classes or not. if we use class, this change will be unnecessary.

@jessetrinity oh right, i'd forgotten - is there an easy way to just run the tests for this one package without having to run them for the entire repo?

But Node "class"es are not real classes

Note that JS doesn't have any sort of "real classes", just class, which is conceptually and functionally identical to ES5 newed constructor functions - anything that's intended to be newed is a "class", altho class produces uncallable constructors (ie, they require new).

TypeScript throws an error for calling classes without new which is a runtime error if target is defined using the class keyword (isn't "actual class" a good name for this case?)

Currently EventEmitter is declared as a class which will produce a TS error for calling it without new, although it's fine in Node.js runtime because its not defined with a class keyword.

@Andy-MS
My question is, should we stop using class for other Node.js "classes" until Node changes its code to use the class keyword?

In that case then anything not declared with class probably shouldn't be typed as one, no

require("events")() fails for me with: TypeError: Cannot set property 'domain' of undefined.

Even if that did work, I think the simplicity of declaring something as a class outweighs the ability to omit the new keyword even if that were supported.

@Andy-MS i'd say if it worked, then typing it to include that would be critical; if it doesn't work, then clearly it's acting as a class even if it can be .called on something.

@jessetrinity fwiw npm install failed for me on node 10 on my mac; is there a specific version of node that this repo requires to work? (i'll rebase and try again, just in case)

Why we have EventEmitter as a class but not Buffer?

I think mostly because no one issues a PR to improve this. I can remember several PRs to change this and that interface to a class to fix concrete issues but no PR like "use classes where applicable". I think that older typescript versions (maybe <2) had also some limitations regarding merging of interfaces, classes and namespaces but I can't remember in detail.

Currently EventEmitter is declared as a class which will produce a TS error for calling it without new, although it's fine in Node.js runtime because its not defined with a class keyword.

NodeJs doc tells it's a class.
@mohsen1 What is the real world usecase to call EventEmitter without new? Sure, for JS point of view it's fine but quite a lot is allowed from JS point of view where typescript tells it's not allowed.

@ljharb Node 10 should be fine. Normally you can run tests from types/foo but in the case of Node you should run the whole things.

Any details on how it is failing? I will have to ask around about Mac.

@jessetrinity compilation errors:

$HOME/.node-gyp/10.10.0/include/node/openssl/ossl_typ.h:107:16
: note: forward declaration of 'struct dsa_st'
typedef struct dsa_st DSA;
               ^
../vendor/libssh2/src/openssl.c:133:14: error: incomplete definition of type 'struct dsa_st'
    (*dsactx)->q = BN_new();
    ~~~~~~~~~^
$HOME/.node-gyp/10.10.0/include/node/openssl/ossl_typ.h:107:16: note: forward declaration of 'struct dsa_st'
typedef struct dsa_st DSA;
               ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.
make: *** [Release/obj.target/libssh2/vendor/libssh2/src/openssl.o] Error 1
gyp

ERR! build error
gyp
ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack
at ChildProcess.onExit ($HOME/.nvm/versions/node/v10.10.0/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:262:23)
gyp ERR!
stack     at ChildProcess.emit (events.js:182:13)
gyp
ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:240:12)
gyp
ERR! System Darwin 17.7.0
gyp
ERR! command "$HOME/.nvm/versions/node/v10.10.0/bin/node" "$HOME/.nvm/versions/node/v10.10.0/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "build" "--fallback-to-build" "--module=$HOME/airlab/repos/DefinitelyTyped/node_modules/nodegit/build/Release/nodegit.node" "--module_name=nodegit" "--module_path=$HOME/airlab/repos/DefinitelyTyped/node_modules/nodegit/build/Release"
gyp
ERR! cwd $HOME/airlab/repos/DefinitelyTyped/node_modules/nodegit
gyp ERR! node -v v10.10.0
gyp
ERR! node-gyp -v v3.8.0
gyp ERR! not ok
[nodegit] ERROR - Could not finish install
[nodegit] ERROR - finished with error code: 1

@ljharb That may be nodegit/nodegit#1490. You should be able to just ignore this and run npm run lint my-package-name directly, which doesn't use that dependency.