zlib: use new.target to check for instantiation#54694
zlib: use new.target to check for instantiation#54694anonrig wants to merge 1 commit intonodejs:mainfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54694 +/- ##
==========================================
+ Coverage 87.33% 87.60% +0.27%
==========================================
Files 650 650
Lines 182832 182843 +11
Branches 35067 35384 +317
==========================================
+ Hits 159670 160180 +510
+ Misses 16421 15929 -492
+ Partials 6741 6734 -7
|
7c70f41 to
e7449c4
Compare
|
@nodejs/tsc this is a semver-major because the removed test is broken. |
e7449c4 to
1af1c3b
Compare
1af1c3b to
e74e917
Compare
| // minimal 2-byte header | ||
| function Deflate(opts) { | ||
| if (!(this instanceof Deflate)) | ||
| if (!new.target) { |
There was a problem hiding this comment.
| if (!new.target) { | |
| if (new.target !== Deflate) { |
if you also want to normalize subclasses
|
Is this more performant? Would it work with subclasses? |
There was a problem hiding this comment.
That seems like something we would want to support, at least using ES6 class syntax
There was a problem hiding this comment.
With the ES6 syntax this test would still pass (except if doing #54694 (comment)).
I guess this change only makes sense if done together with #54676, since this is about detecting when new is used or not.
mcollina
left a comment
There was a problem hiding this comment.
I'm -1, we should keep supporting the removed test
Referring to @nicolo-ribaudo's comment: #54676 (comment)