events: deprecate EventEmitter.defaultMaxListeners getter/setter

[devsnek]

In preparation for upcoming ESM interop and to remain consistent with
other getters/setters being removed

Refs: #18131

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

events

[devsnek]

CI https://ci.nodejs.org/job/node-test-pull-request/13105/

[vsemozhetbyt]

.setDefaultMaxListeners -> .getDefaultMaxListeners ?

[vsemozhetbyt]

{Number} -> {number} (primitives are lowercased).

[vsemozhetbyt]

A nit: EventEmitter. API chapters were out of ABC-order. We can now fix this, so that

EventEmitter.listenerCount
EventEmitter.defaultMaxListeners
EventEmitter.getDefaultMaxListeners
EventEmitter.setDefaultMaxListeners

will become

EventEmitter.defaultMaxListeners
EventEmitter.getDefaultMaxListeners
EventEmitter.listenerCount
EventEmitter.setDefaultMaxListeners

But this can be done in a separate PR if this is more appropriate)

[TimothyGu]

I thought we had decided that only the setter function needs to be exported, while the live binding would suffice in place of the getter?

[devsnek]

@TimothyGu dunno, that feels a bit inconsistent in terms of behavior and seeing as we have crypto.getFips, i thought this seemed better.

[devsnek]

@nodejs/core anyone wanna review this?

[mcollina]

I’m not necessarily on board with a runtime deprecation for this.
Is this popular and used? This might be around some old code snippets.

Also, these values are somewhat process-altering and “against” the immutability of ESM.

I’m more leaning towards removing these completely. Do we have an usecase for these to exists?

[benjamingr]

Doing a runtime deprecation like this is too soon IMO - we can deprecate it in the docs rather than in runtime and deprecate it in runtime later.

[ChALkeR]

This looks heavily used in the ecosystem, so runtime-deprecation in 10.x could be too disruptive, given that there is no fallback yet.

+1 for doc-deprecation with --pending-deprecation support.
Also, the new methods should probably be backported to ease the migration path.

[ChALkeR]

On a second look, most of the grep results most likely come from re-packaging copies of events module implementations.

Raw grep gist: https://gist.github.com/ChALkeR/127ea95c65297d533f37ae91391e9188.

Filtered grep (to exclude events copies) gist: https://gist.github.com/ChALkeR/583429e159834e9e57562960f87f3c88

The filtered results are significantly smaller.

In fact, I'm ok with runtime-deprecating based on that, but I would still prefer pending-deprecation first, and getting the new getters/setters backported. It's a documented public API, and it should go through doc-deprecation first if there are no strong reasons to rush it.

[devsnek]

for a docs only, the only difference to this is that the setter and getter wouldn't be wrapped in deprecate right?

[ChALkeR]

@devsnek Take a look at https://github.com/nodejs/node/pull/18415/files#diff-cc63475f116262e9cbea02374e344c01R119

[jasnell]

Yep, a docs-only deprecation means listing a new deprecation code in deprecations.md and adding note to the API doc indicating that the thing has been deprecated. Super simple.

stale

[mcollina]

LGTM

[guybedford]

Is this still relevant if we can make Proxies work?

[mcollina]

This is relevant anyway for any named export solution.

[guybedford]

But with proxies being supported, setters and getters can be supported fine on named exports right? The only potential issue is surely how to set a named export from the ES module side?

[benjamingr]

Honestly I find getters and setters on named exports very confusing anyway. Wrapping it with proxies would work but be confusing anyway, and deprecating them in favor of functions that are on root exports seems reasonable to me.

[devsnek]

@nodejs/tsc i assume one of you people need to review this?

[addaleax]

I don’t think I’m on board with ever deprecating this. This has been around for years, and there’s nothing wrong with using it.

If ESM compatibility is the concern here, then yes, let’s add the getter/setter functions and say that that’s what you’re supposed to use then.

[addaleax]

Making -1 explicit

[targos]

I'm with @addaleax on this one. require('events') returns a class, not a namespace. I don't think it makes sense anyway to support named imports on EventEmitter's properties.

[jkrems]

For clarity, in ESM world events would then only have a default export which is the EventEmitter class and no named exports at first? That sounds reasonable to me.

[targos]

in ESM world events would then only have a default export which is the EventEmitter class and no named exports at first?

That's the idea, yes.

[jdalton]

With the named exports+live binding for builtin modules PR (unfiltered for setters) I believe both getters/setters are supported.

import events, { defaultMaxListeners } from "events"

console.log(defaultMaxListeners)
// 10
console.log(events.defaultMaxListeners)
// 10
events.defaultMaxListeners = 4
console.log(defaultMaxListeners)
// 4

[devsnek]

If ESM compatibility is the concern here, then yes, let’s add the getter/setter functions and say that that’s what you’re supposed to use then.

...thats basically deprecating it anyway? having a well designed api is part of having an api, and just as important as the functionality. if we're saying to use the shiny new x instead of y, we should also explicitly say don't use y

in ESM world events would then only have a default export which is the EventEmitter class and no named exports at first?

when i added this in my named exports pr i got pushback for "singling out events" which led to this pr. i'm fine with only having the default export as long as i'm actually able to implement it without pushback

[addaleax]

If ESM compatibility is the concern here, then yes, let’s add the getter/setter functions and say that that’s what you’re supposed to use then.

...thats basically deprecating it anyway? having a well designed api is part of having an api, and just as important as the funtionality

Deprecating means that something should not be used. However, there is no reason not to use this in CJS, right?

We don’t do CJS users a favor by deprecating this (esp. not with --pending-deprecation), and new ESM code also won’t have to worry about a deprecation, because they cannot use the deprecated mechanism anyway.

when i added this in my named exports pr i got pushback for "singling out events" which led to this pr. i'm fine with only having the default export as long as i'm actually able to implement it without pushback

We could also leave out getter/setter properties for these cases, and leave the corresponding get/set functions in place, right?

(I mean, I think only exporting EventEmitter is a good thing. But that’s not really my call I guess. ¯\_(ツ)_/¯)

[devsnek]

However, there is no reason not to use this in CJS, right?

i don't see any distinction between this pr and #18335 to that respect. additionally migrating between esm and cjs is already enough of a headache without trying to figure out getters and setters vs methods that get and set and in which cases they were migrated and in which cases they weren't etc

[jdalton]

Setters on builtin modules aren't necessarily an ESM problem anymore folks.
This PR can be closed without impacting Node's ESM plans.

[addaleax]

i don't see any distinction between this pr and #18335 to that respect.

crypto.fips was introduced in Node 6 (2016), this property was introduced in Node v0.11.2 (2013), and generally this is much, much more widely used.

[giltayar]

As @targos said:

require('events') returns a class, not a namespace. I don't think it makes sense anyway to support named imports on EventEmitter's properties.

This means that this line, @jdalton, ...

import events, { defaultMaxListeners } from "events"

...makes no sense in terms of ESM, as "events" exports only the class EventEmitter. One cannot use ESM import to extract a static field from a class. ESM import syntax is not destructuring. It's a syntax for importing specific things from a module. Thus, we cannot use it to extract defaultMaxListeners. Even if it somehow worked in CJS.

So IMHO, @targos's point makes this PR not needed.

[weswigham]

One cannot use ESM import to extract a static field from a class. ESM import syntax is not destructuring. It's a syntax for importing specific things from a module

The static side of a class is not really different than any other object one could set module.exports to equal. The object just also happens to be a constructor function.

[giltayar]

@weswigham:

The static side of a class is not really different than any other object one could set module.exports to equal. The object just also happens to be a constructor function.

True! And in CJS, you would be correct, as you can use destructuring to access that: const {defaultMaxListeners} = require('events').

But the deprecation in this PR, if I understood correctly, is for ESM times. But in ESM, we're only default-exporting the class, and doing import {defaultMaxListeners} from 'events' will not destructure the class fields. The syntax looks the same, but isn't: import {...} cannot destructure an object default value.

[weswigham]

But the deprecation in this PR, if I understood correctly, is for ESM times. But in ESM, we're only default-exporting the class, and doing import {defaultMaxListeners} from 'events' will not destructure the class fields.

No, there's an open PR to allow exactly that (pulling named exports off whatever the exported object is, in addition to a default). There was originally an issue with not knowing if fields/getters/setters would work, but it has since been resolved, as @jdalton keeps saying. This property doesn't need to be deprecated for any technical reason any more.

[jdalton]

@giltayar, @weswigham
The example I used is from working code of #18131.

[devsnek]

@jdalton i just want to be clear that i'm not comfortable landing named exports if you can import { defaultMaxListeners } from 'events'

whether its by this pr or a condition in the loader to only expose a default export doesn't matter very much to me.

[weswigham]

@devsnek how is it meaningfully different to import defaultMaxListeners over readFile? It really shouldn't be; they're presented in essentially the same way. It's all well and good to say you don't like fields/getters/setters and that you want to deprecate them, but they are by no means required to be gone to have an esm named exports shim that works as expected.

[giltayar]

No, there's an open PR to allow exactly that (pulling named exports off whatever the exported object is, in addition to a default).

So maybe the problem is with #18131. We're doing split-personality here. In one personality, we want a default export: import fs from 'fs'. In the other, we want named exports from the same default value fs.

We're thinking CJS destructuring, when it actually isn't applicable to ESM-think.

I believe we should decide, per module, if we're exporting a default value or some named exports. Yes, the spec supports exporting both, but I don't believe the idea was to export the properties of the default object as named exports.

And if we decide, as I believe we should, to export all the functions in fs as named exports, ESM still has a syntax that allows us to use them as an object. i.e. import * as fs from 'fs'.

To sum up: we should decide on a module by module basis whether we want the exports to be exported using default, or using named, but never both. In fs, it's reasonable to choose named exports, and in events, it's reasonable to choose default export.

[devsnek]

@weswigham the point of named exports is not to just blindly copy an object to a module namespace, its to expose a well designed api to esm users. imagine the api that would be exposed if we rewrote every builtin with es module syntax, and thats what should be exposed.

[giltayar]

@weswigham - to answer your question to @devsnek: because, in ESM-think, what is exported from fs should be a set of named export functions, and what is exported from events should be a default value of the class.

[devsnek]

i'm going to close this pr and use it as my reasoning for special casing named exports. thanks to all involved for a productive discussion

[weswigham]

its to expose a well designed api to esm users.

Stop using the esm transition as an excuse to rewrite APIs. Either rewrite them on their own merits for all users, or leave them be. It's not required from a technical perspective (just like that nonsense of automatically filtering underscore prefixed names).