search

No9 / harmon

middleware for node-http-proxy to modify the remote website response with trumpet
Other
424 stars 62 forks source link

Doesn't work nowadays? Examples and tests fail #46

Open motin opened 6 years ago

motin commented 6 years ago

This project looks exactly like what I was looking for, but I got nothing to work.

It seems to be a general issue, with CircleCI on a fork of the current master failing as well: https://circleci.com/gh/motin/harmon/1

npm test

> harmon@1.3.2 test /home/ubuntu/harmon
> node test/host.js

BODY: <html><head></head><body><div class="a">Nodejitsu Http Proxy</div><div class="b">&amp; Frames</div></body></html>

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: '<html><head></head><body><div>Harmon Middleware</div><div>+ Trumpet</div></body></html>' == '<html><head></head><body><div class="a">Nodejitsu Http Proxy</div><div class="b">&amp; Frames</div></body></html>'
    at IncomingMessage.<anonymous> (/home/ubuntu/harmon/test/host.js:79:14)
    at emitNone (events.js:72:20)
    at IncomingMessage.emit (events.js:166:7)
    at endReadableNT (_stream_readable.js:905:12)
    at nextTickCallbackWith2Args (node.js:441:9)
    at process._tickCallback (node.js:355:17)
npm ERR! Test failed.  See above for more details.

npm test returned exit code 1

I tried node versions 8, 4 and 0.10 to no avail.

No9 commented 6 years ago

Thanks @motin There's an issue with http-proxy@1.17.0. I don't have the bandwidth to investigate if this is a bug in 1.17.0 or if we should modify this package.

But if you need to make forward progress I would suggest downgrading to 1.15.1 as specified in the test. I've tightened the package.json so the tests pass https://github.com/No9/harmon/commit/e52b83d9ae0923bd33f8556e7a34b77ec05f56d6

Happy to take a PR if the investigation points to it being a need to upgrade this package

No9 commented 6 years ago

Additional investigation shows that the harmon tests pass up to @1.16.2

motin commented 6 years ago

Thanks @No9 for pointing this out. Btw, shouldn't some of these devDependencies be actual dependencies? Currently when including harmon in another project, neither connect nor http-proxy gets installed.

No9 commented 6 years ago

Hey @motin The lack of inclusion of connect and http-proxy is by design - harmon follows the middleware pattern so it should be possible to use it be usable outside of those two frameworks and I prefer libraries over frameworks but YMMV :)

jcrugzz commented 6 years ago

Hmm it seems like this broke when we stopped explicitly calling res.writeHead. I thought the internal call by node itself would allow it to still work but I guess not. The reason we switched to just assigning statusCode and allowing it to implicitly writeHead was to prevent headers were already sent errors from being thrown in certain edge cases. I'd like to see if harmon could work without overriding res.writeHead. any thoughts @No9?

andrebautista commented 6 years ago

@jcrugzz Yes! Maybe we should have coordinated on this one 😝

@No9 (written before I refreshed the page and saw the previous response) After some investigation I believe I found the culprit of this bug. It has to do with http-proxy not using writeHead in their latest build http-proxy #953 fix I can patch it up but I need a little bit of guidance.

The issue is that res.write is running before res.writeHead therefore res.isHtml isn't being set prior to rewriting the body and replacing the content through trumpet. I'm guessing this is due to res.writeHead not being explicitly called by http-proxy anymore. Is there somewhere where I can configure what order these events fire in?

computersrmyfriends commented 6 years ago

Any updates on this? Is this issue fixed now or should an old version of http proxy be used?

leveneg commented 6 years ago

I've opened #50 to address the issue with http-proxy. Feedback definitely welcome!

No9 commented 6 years ago

@leveneg Thanks for this - I'll take a look this week and provide feedback