chore: remove node-http-proxy and instead install package from npm and patch it #29499
Conversation
| -# Changelog | ||
| - | ||
| -All notable changes to this project will be documented in this file. | ||
| - | ||
| -The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) | ||
| -and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | ||
| - | ||
| -Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). | ||
| - | ||
| -## [v1.18.1](https://github.com/http-party/node-http-proxy/compare/1.18.0...v1.18.1) - 2020-05-17 | ||
| - | ||
| -### Merged |
There was a problem hiding this comment.
@AtofStryker Is this patch supposed to have all of this information in here? I thought it's typically a few lines.
There was a problem hiding this comment.
I've been trying to figure this out and I think its our friend .yarnclean https://github.com/cypress-io/cypress/blob/develop/.yarnclean#L53. going to turn it off to develop the patch then reenable it
There was a problem hiding this comment.
@jennifer-shehane it was the yarnclean. should be fixed now
AtofStryker
force-pushed
the
chore/update_node_proxy_patch
branch
from
6975343
to
61c78c5
Compare
There was a problem hiding this comment.
@AtofStryker Looks like this test is reliably failing across chrome/firefox/webkit
I wonder if its because I nohoisted the package. looking into it |
AtofStryker
force-pushed
the
chore/update_node_proxy_patch
branch
3 times, most recently
from
017059b
to
c1ab750
Compare
Passing run #55373 ↗︎
Details:
Review all test suite changes for PR #29499 ↗︎ |
|||||||||||||||
…d patch it [run ci]
AtofStryker
force-pushed
the
chore/update_node_proxy_patch
branch
from
c1ab750
to
486a9b0
Compare
|
@jennifer-shehane turns out the enum doesn't actually exist on the socket object so it was evaluating the expression to false. I have updated the patch to be: if (!res.upgrade && socket.readyState === "open" && !socket.destroyed)which should work as expected |
| proxyReq.on('response', function (res) { | ||
| // if upgrade event isn't going to happen, close the socket | ||
| - if (!res.upgrade) { | ||
| + if (!res.upgrade && socket.readyState === "open" && !socket.destroyed) { |
There was a problem hiding this comment.
@AtofStryker Is this case sensitive? Would be a bit ridiculous if it is, but they are in all caps in the docs.
There was a problem hiding this comment.
which docs are you referring to? Are you saying the readyState is in caps?
There was a problem hiding this comment.
If this is a Websocket that conforms to MDN standards, WebSocket.readyState is a number that corresponds to WebSocket.OPEN, etc. It looks like socket-io also conforms to this. What's creating this socket, and what type is it? The stream() method here isn't called directly by http-proxy, so it must be getting called from packages/server, or as an internal node lifecycle type process.
readyState also doesn't seem to be a property on duplex streams, which is what a node http/https socket is.
There was a problem hiding this comment.
However, socket is implied to be a duplex stream (I believe) in checkMethodAndHeader with the use of .destroy(), which is a member of Duplex but is not a member of WebSocket... what the heck is this value? 🤔
There was a problem hiding this comment.
There was a problem hiding this comment.
Logging the value of socket here during the websockets_spec system test, and it looks like a decorated Duplex stream - it has stream related properties, like destroyed, _readableState, and _writeableState, but it also has properties describing a server:
{
...restOfProperties,
server: {
...somePrivateProps,
allowHalfOpen: boolean
connectionsCheckingInterval: number
headersTimeout: number
highWaterMark: number
...etc
}
}It does not, however, have a readyState property, so I'm not certain this conditional will ever evaluate as true.
There was a problem hiding this comment.
You need to log the value of socket.readyState directly. It's not enumerable and it returns undefined for Object.getOwnPropertyDescriptor(socket, 'readyState') but when accessed directly:
console.log('readyState is enumerable:', socket.propertyIsEnumerable('readyState'));
// => readyState is enumerable: false
console.log('description:', Object.getOwnPropertyDescriptor(socket, 'readyState'));
// => description: undefined
console.log('socket.readyState', typeof socket.readyState, socket.readyState);
// => socket.readyState string open
There was a problem hiding this comment.
It seems like this is the Socket.readyState property
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
Additionally can close cypress-io/node-http-proxy#1 and #29453 since we will no longer be using the fork. The fork is only one commit ahead of the base release, so it makes more sense on our end to patch the package (with the changes suggested in cypress-io/node-http-proxy#1 as well) so we can more easily maintain changes to the node-http-proxy
Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?