zlib: runtime deprecate initializing without new qualifier

[anonrig]

I wanted to open this pull-request as a draft to get a feedback from @nodejs/tsc the possibility of runtime deprecating new qualifier in the next major.

If we are OK with this change, I can open another pull-request before landing this to documentation-only deprecate it with the next release, before the next major.

[jasnell]

While I am absolutely in agreement with this in spirit, I don't think we will ever actually be able to remove the use of the non-new qualified variant. There is simply too much code out there that is using these classes without the new keyword. If I recall correctly, we tried to make this kind of change before to some other classes and it end up breaking a lot of people. So we need to be very careful on how we move forward, if we do choose to.

[anonrig]

There is simply too much code out there that is using these classes without the new keyword.

Without deprecating them it's impossible to get rid of them. Runtime deprecation will create noise, but that's why we will make it a semver major. I personally don't see a downside of deprecating these. I think the timeline to remove this feature, is a different discussion.

[mcollina]

I don't see a good reason to put the community under this kind of stress.

[marco-ippolito]

+1 as I think this is necessary housekeeping, and not necessarily make it eol. As per standard, it needs to be doc deprecated first and then runtime deprecated.

[anonrig]

I don't see a good reason to put the community under this kind of stress.

@mcollina If you are open to discussion and don't have hard no on this particular change, here's some:

• this has a huge impact on ecosystem and potentially leads the ecosystem to use modern javascript syntax
• the performance implications on initializing these classes (since there won't be ReflectConstruct and instanceof checks)
• modernization of the node.js codebase. eventually zlib code can be quite simplified if we used modern syntax. i recently did a similar work on cloudflare workers, and it was an eye opening experience for me.

[nicolo-ribaudo]

@anonrig I was thinking about this PR (I don't know why, I don't even remember how I found it in the first place 😅), and if your goal is purely to move to ES6 classes while preserving behavior, you might be interested in this code snippet:

// extending this function allows providing a custom "this" value for classes
function Identity(x) { return x }
Identity.prototype = Object.prototype;

class InternalBaseClass extends Identity {
  constructor(param1, param2, customThis) {
    super(customThis);
  }
}

class InternalSubClass extends InternalBaseClass {
   constructor(opts, customThis) {
     super(opts, DEFLATE, customThis);
   }
}

// Replace the InternalSubClass constructor with one that can also be called without new
export function PublicSubClass(opts) {
  if (!(this instanceof InternalSubClass)) {
    return new InternalSubClass(opts, undefined);
  }
  return new InternalSubClass(opts, this);
}
Object.setPrototypeOf(PublicSubClass, Object.getPrototypeOf(InternalSubClass));
PublicSubClass.prototype = InternalSubClass.prototype;
PublicSubClass.prototype.constructor = PublicSubClass;

I didn't test performance.

Once Node.js ships decorators, it could become

// extending this function allows providing a custom "this" value for classes
function Identity(x) { return x }
Identity.prototype = Object.prototype;

class InternalBaseClass extends Identity {
  constructor(customThis, param1, param2) {
    super(customThis);
  }
}

export @allowCall class PublicSubClass extends InternalBaseClass {
   constructor(customThis, opts) {
     super(customThis, opts, DEFLATE);
   }
}

function allowCall(Class) {
  function Callable(...args) {
    return new Class(this instanceof Class ? this : undefined, ...args);
  }
  Object.setPrototypeOf(Callable, Object.getPrototypeOf(Class));
  Callable.prototype = Class.prototype;
  Callable.name = Class.name;
  Callable.prototype.constructor = Callable;
  return Callable;
}

[anonrig]

I don't see a good reason to put the community under this kind of stress.

@mcollina Since the documentation-only deprecation landed, I want to move forward with this pull-request. I've tried to address your block on my comment: #54676 (comment). Did you had a chance to think over this, and maybe remove the block? We can always add tsc-agenda but I prefer to communicate before doing so.

If any other @nodejs/tsc members are interested in reviewing, we need 2 TSC approvals to land this since this is a semver-major pull-request.

[codecov]

Codecov Report

Attention: Patch coverage is 21.42857% with 22 lines in your changes missing coverage. Please review.

Project coverage is 87.59%. Comparing base (143aca7) to head (c9307a2).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/zlib.js	21.42%	22 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54676      +/-   ##
==========================================
- Coverage   87.61%   87.59%   -0.02%     
==========================================
  Files         650      650              
  Lines      182945   182964      +19     
  Branches    35392    35393       +1     
==========================================
- Hits       160280   160267      -13     
- Misses      15917    15955      +38     
+ Partials     6748     6742       -6     

Files with missing lines	Coverage Δ
lib/zlib.js	95.21% <21.42%> (-2.98%)	⬇️

... and 21 files with indirect coverage changes

[mcollina]

To do these kind of things, we need a clear analysis of how much these APIs are used in the wild, and try to help the maintainers to deal with it before the problem reaches them.

This was exceptionally hard for Buffer(), and while it was being cause of a lot of vulnerabilities, it took years for the community to move past it.

@anonrig I don't see any way this helps the ecosystem, and you didn't
mention anything either. I consider forcing the ecosystem to migrate to modern syntax a net negative, mostly because it's work we force on them.

I concur that it help us, but I don't see a strong motivation for this either. Zlib is a very stable part of core.