Closed patrick-steele-idem closed 8 years ago
Also, I just wanted to add that after digging in the history some more I realized that Node.js never had OutgoingMessage.prototype.flush
(just io.js). I thought maybe it was added in Node.js 0.12 but that is not the case. Given that OutgoingMessage.prototype.flush
only ever existed in io.js it feels even more sad that flush()
has been ruined for Node.js 4.0 :(
I'm going to move to close this. This is how we deprecate things. You can use --no-deprecation
(I think that's the right one) if you don't want warnings printed.
Now that someone has committed the bad code I agree with you it has to be deprecated properly but it is still causing a problem. ..
The problem is that the original addition of OutgoingMessage.prototype.flush
introduced both req.flush()
and res.flush()
. Long before Node.js introduced the flush()
method, the popular compression middleware was already introducing a res.flush()
method and now we have a conflict since we have the same method for different purposes. I realize that issues caused by monkey-patching in third-party libraries is not something the Node.js core team should have to deal with which is why I asked the following question:
I don't know the right answer here, but just wanted to start the discussion to see how soon we can fix this inconsistency (Node.js 5.0? If so, how long will that be?) and to make more people aware of the issue. Is there a lot of code in the wild that was using flush() for the purpose of flushing headers on an OutgoingMessage?
I would like to see the OutgoingMessage.prototype.flush
be completely removed or to make it a no-op. For now, the workaround is to do the following:
OutgoingMessage.prototype.flush = function() {}
// Or: delete OutgoingMessage.prototype.flush;
...And hope that any code has already moved to flushHeaders()
:)
We have to put in a hack to solve the problem before we can move to Node.js 4.x because of a problem that was introduced in io.js before the merge. Definitely not a major issue though. I'm okay with you closing this issue, but do please let me know when we could expect the deprecated OutgoingMessage.prototype.flush
to be removed. Thanks for reading and I apologize for the long text.
Here is the code:
res.flush: https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L675-L686
util.deprecate: https://github.com/nodejs/node/blob/master/lib/internal/util.js
This just means the library you are using hasn't been updated yet. We've long issues deprecation warning this way and will continue to. If printing to stdio
causes issues for you, you need to run node with --no-deprecation
.
The property will likely be deleted in a year or two, for now, just check the node version.
As for the original addition, that well over a year ago in 0.11.14: https://github.com/nodejs/node/commit/bd24ab2bd788a196adaac316db20cde2c8c8e14d. Not much we can do in retrospect.
tldr; Implementing
req.flush()
and having it log a deprecation message to the console is causing problems with code that expectsflush()
to be implemented correctly.I understand that
OutgoingMessage
used to have aflush()
method that was poorly named. This was later fixed by renaming it toflushHeaders()
. However, instead of stopping there, a stub for theflush()
method was put in with the following implementation:See commit: https://github.com/nodejs/node/commit/b2e00e38dcac23cf564636e8fad880829489286c
I understand that this was done to maintain backwards compatibility, but it is also preventing libraries from actually using the
flush()
method for the correct purpose. It's my opinion that we should have just deleted the oldflush()
method (it was a major release after all), but it's a little late for that now....This change is causing a problem for libraries that check if
flush()
is implemented and want to use it for the purpose of actually flushing buffered data. You are probably thinking thatflush()
was never used for that purpose, but that is not true...For example, the compression module monkey patches theresponse
stream to include aflush()
method for the expected and correct purpose. See:https://github.com/expressjs/compression/blob/c2af8bd8d5cec3577b40d61859ca3a0467052ded/index.js#L61-L65
The marko templating engine is an asynchronous and streaming templating engine that is designed to
flush()
bytes to the underlying stream whenever an asynchronous fragment is completed. It works by conditionally checking if the underlying stream has aflush()
method and it will then call theflush()
method if found. See:https://github.com/marko-js/async-writer/blob/5ef039139041fb40aadcdc0b7a10eb7d3d3a48b5/lib/AsyncWriter.js#L494-L496
This works great when using the gzip compression middleware that monkey patches the
flush()
method but when rendering to anOutgoingMessage
stream with the compression middleware we are seeing a deprecation message being written to the console :disappointed:As a result, users of
marko
are reporting the problem that a deprecation warning is being written to the console: https://github.com/marko-js/async-writer/issues/3I understand the dilemma, but without knowing if a stream is really flushable we are seeing problems when trying to use the
flush()
method for the purpose that thecompression
middleware intended.I don't know the right answer here, but just wanted to start the discussion to see how soon we can fix this inconsistency (Node.js 5.0? If so, how long will that be?) and to make more people aware of the issue. Is there a lot of code in the wild that was using
flush()
for the purpose of flushing headers on anOutgoingMessage
?