-
Notifications
You must be signed in to change notification settings - Fork 6.2k
pathProxy.ts: Implement --proxy-path-passthrough #2563
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.
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
Conversation
Even with all of this it doesn't work perfectly...
the sockjs-node connection from webpack doesn't use create-react-app's homepage and so is still absolute. I think I'll implement this and also the Referrer solution I referred to in #2222. It's the only way. Very unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Great work 🎉
Do we want to write tests for this?
I tested locally (following the docs) and it works! 🌮
Unfortunately Referrer isn't set on websocket requests and not always set in general so not something we can rely on. create-react-app will have to fix that to have webpack-dev-server properly work through our proxy. Super unfortunate.
920d914 to
4ff5443
Compare
Unfortunately Referrer isn't set on websocket requests and not always set in general so not something we can rely on.
create-react-appwill have to fix that to have webpack-dev-server properly work through our proxy. Super unfortunate.
Do you plan to open up an issue on the create-react-app? (If there isn’t one already)
I also converted back to a draft, didn't know I could do that.
Tests nearly done! but there's some bizarre fd leak somewhere I need to fix still :(
e026aee to
bf97d37
Compare
This should be good now @jsjoeio!
omg, i see now, it was leaked-handles. confusing me! updating the commit message.
b591fa0 to
5723c7a
Compare
@nhooyr great work and great tests! Left one minor nit/question about an any TS type but non-blocking.
Is this PR intentionally fixing other issues? I see stuff related the the dark mode favicon and the heartbeat error that looks like you fixed. Just wanted to make sure! Otherwise, I can approve
Wooo this is fabulous; the tests are 🔥
For some history on leaked-handles: if I recall correctly I had issues with code-server or one of the child processes not being killed with a SIGTERM or something due to the socket hanging open, I think. So I added this, tracked down the leaked handles, and it resolved the issue.
Or actually it might have been keeping the tests themselves from finishing? I don't quite recall. Something was hanging somewhere.
With all that said I feel a little more confident if it stayed in but I don't feel super strongly either way.
Is this PR intentionally fixing other issues? I see stuff related the the dark mode favicon and the heartbeat error that looks like you fixed. Just wanted to make sure! Otherwise, I can approve
Yea I screwed up this branch somehow. The favicon commits are unrelated but see my comment re the heartbeat changes.
With all that said I feel a little more confident if it stayed in but I don't feel super strongly either way.
Happy to leave it in if we can modify the message to indicate it's from that package. Otherwise, it's just confusing if the leak is elsewhere.
And the concerns surrounding it. Closes #2485
express requires all 4 arguments to be declared for a error handler. It's very unfortunate that our types do not handle this.
The goal is to remove supertest as it does not support typescript well and there's really no good reason for the dependency. Also no websocket testing support.
This had me very confused for quite a while until I did a binary search
inspection on route/index.ts. Only with the heart.beat line commented
out did my tests pass without leaking.
They weren't leaking fds but just this heartbeat timer and node of
course prints just fds that are active when it detects some sort of leak
I guess and that made the whole thing very confusing. These fds are not
leaked and will close when node's event loop detects there are no more
callbacks to run.
no of handles 3
tcp stream {
fd: 20,
readable: false,
writable: true,
address: {},
serverAddr: null
}
tcp stream {
fd: 22,
readable: false,
writable: true,
address: {},
serverAddr: null
}
tcp stream {
fd: 23,
readable: true,
writable: false,
address: {},
serverAddr: null
}
It kept printing the above text again and again for 60s and then the
test binary times out I think. I'm not sure if it was node printing the
stuff above or if it was a mocha thing. But it was really confusing...
cc @code-asher for thoughts on what was going on.
edit: It was the leaked-handles import in socket.test.ts!!!
Not sure if we should keep it, this was really confusing and misleading.
5723c7a to
c32d8b1
Compare
Fixed the branch so the favicon changes aren't part of the diff anymore! Was based on a very old version of master sorry!
Happy to leave it in if we can modify the message to indicate it's from that package. Otherwise, it's just confusing if the leak is elsewhere.
Looks like we can't :(
And https://github.com/myndzi/wtfnode is the preferred utility anyway and has much better output. Opening a PR to switch.
Also unfortunate and confusing thing about leaked-handles is that it's usage is global as it begins on import instead of letting you call a method to enable. I guess you could require as needed but dynamic requires are a little more annoying. wtfnode just has a global function you call to begin the dumping.
Oo and we can configure the output further if ever needed! https://github.com/myndzi/wtfnode#configuring-logging
See my comments at coder#2563 (comment)
See my comments at #2563 (comment)
See my comments at #2563 (comment)
Awesome, thanks for the reviews guys!
See my comments at #2563 (comment)
See my comments at #2563 (comment)
See my comments at #2563 (comment)
Closes #2222