stream: avoid caching prepend check #8018
Conversation
nodejs-github-bot
added
the
stream
|
Ugh. Dang. Ok. |
| prependListener = function prependListener(emitter, event, fn) { | ||
| function prependListener(emitter, event, fn) { | ||
| // sadly this is not cacheable as some libraries bundle their own | ||
| // event emitter implimentation with them |
|
yeah libraries compiled as UMDs for node are the problem case this would solve |
| this.push(null); | ||
| }; | ||
|
|
||
| var w = new Writable(); |
There was a problem hiding this comment.
Can you use const here, and below.
There was a problem hiding this comment.
yes I can, thanks
|
LGTM I think, CI: https://ci.nodejs.org/job/node-test-commit/4457/ |
|
Still LGTM with the latest commits if CI is green. |
|
LGTM |
|
ok there was a linting error but I fixed that |
|
Hopefully final CI then: https://ci.nodejs.org/job/node-test-commit/4460/ (and below) |
|
ok I'm heading out the door, but if that looks good i can squash that latter tonight |
|
are those failures something I should worry about ? |
|
I don’t think so, but maybe the FreeBSD failure (test-timers-same-timeout-wrong-list-deleted from #7827) is interesting to @erinspice @misterdjules? |
|
New CI to be sure: https://ci.nodejs.org/job/node-test-commit/4472/ |
|
More flaky failures unfortunately. They appear unrelated. |
This removes the cached check for EE.prototype.prependListener because we can't have nice things. More specifically some libraries will bundle their own event emitter implementation.
calvinmetcalf
force-pushed
the
no-caching-prepend-check
branch
from
ccea2e2 to
9e0f7f4
Compare
|
ok rebased it then |
|
CITGM looks good with only expected failures. |
|
LGTM |
This removes the cached check for EE.prototype.prependListener because we can't have nice things. More specifically some libraries will bundle their own event emitter implementation. PR-URL: #8018 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
|
Landed in 774146d |
This removes the cached check for EE.prototype.prependListener because we can't have nice things. More specifically some libraries will bundle their own event emitter implementation. PR-URL: #8018 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
|
should this be backported? |
|
No, the original change that introduced this issue was not backported to v4. |
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesno but it failed before my change
Affected core subsystem(s)
@nodejs/streams
Description of change
This removes the cached check for EE.prototype.prependListener because we can't
have nice things. More specifically some libraries will bundle their own event
emitter implementation.