Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
motin opened this issue Apr 26, 2018 · 9 comments
Open

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

motin opened this issue Apr 26, 2018 · 9 comments
Labels
help-wanted

Comments

@motin
Copy link

motin commented Apr 26, 2018 •
edited
Loading

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


> [email protected] 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
Copy link
Owner

No9 commented Apr 30, 2018

Thanks @motin
There's an issue with [email protected].
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 e52b83d

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

@No9
Copy link
Owner

No9 commented Apr 30, 2018

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

@motin
Copy link
Author

motin commented Apr 30, 2018

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
Copy link
Owner

No9 commented May 1, 2018

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 :)

@steph643 steph643 mentioned this issue Jun 18, 2018
Open
@jcrugzz
Copy link

jcrugzz commented Jul 23, 2018

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
Copy link

andrebautista commented Jul 23, 2018 •
edited
Loading

@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
Copy link

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

renehamburger added a commit to renehamburger/blinx.js-proxy that referenced this issue Sep 12, 2018
@renehamburger
Fix version of http-proxy due to bug
ac3f8ea 
See No9/harmon#46
@leveneg
Copy link
Contributor

leveneg commented Oct 5, 2018

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

@No9
Copy link
Owner

No9 commented Oct 8, 2018

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

@unixfox unixfox mentioned this issue May 19, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help-wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@No9 @computersrmyfriends @jcrugzz @motin @andrebautista @leveneg