Fix basic auth #1413
Fix basic auth #1413
Conversation
Conflicts: request.js
Check for Auth instance value in basic-auth tests Closes request#1409 Closes request#1412
|
@nylen I think only the last commit here needs review, since @seanstrom reviewed the rest of the commits. |
Some logic was pulled out of init to allow for unix socket support via asynchronous connection attempts. Now that these async connections are gone, this unnecessary and confusing split is no longer needed. See #1186 I apologize to all of your future git blame attempts.
|
@simov this looks awesome, sorry I couldn't help review sooner (like @nylen I'm in the middle of switching jobs). Love the move to modularize but I'm still super wary to give every plugin a blank check to monkey patch the request object. Understanding the request state and chasing bugs around is going to be much more difficult for everyone after a change like this (and it was already pretty difficult before). Sounds like I'm late to the review party, so don't let me get in the way of merging this. But I'd still like us to look into how we can build some structure/rules into this plugin system to help protect contributors from introducing conflicts all willy-nilly :) |
|
Think about these 4 request modules as Express middlewares, where |
|
Anyone I think we should merge this soon, @nylen ? |
|
The express middleware analogy might be where I'm having trouble with this then. The idea of functions sending request/response state down a middleware waterfall is very different from plugins all modifying the same parent object. While the potential for conflicts are present in both, middleware is very linear (one direction) and restricted (req/res state only) while this new system goes around in different loops & paths (redirects, gzip, etc.) and has no restrictions (each can modify whatever on the request object). I'm not advocating for more work, just that we do that work in a thoughtful way. This is a really good first step, and I'd much rather see this than nothing. But I've been burned by this thinking almost every time, where some half-measure is implemented as a first step but the second, third, etc. steps never actually happen. I'm only asking that we spend some time now or immediately after merging to think about how this system could be improved to be safer and more maintainable. |
|
I know about the state problem, in fact everyone does, and I was the one advocating for proper capsulation in the first two modules #1360 and #1366 Also I'm not saying that I won't continue with the refactoring, I'm just presenting it at steps that are more easy to review. So the idea is pretty simple - I have a huge amount of code in one file (around 2000 lines at start), then the first step is to move code out of it, before I start to see a pattern. Btw if you take a look at these 4 new files (probably except the redirect one) they actually look pretty logical, and are very easy to read, so guess what, even this simple step took a few hours to make. My point here is that I think about this stuff while doing it, and that's completely different than only thinking about it. Unfortunately design patterns and principles are not applicable in every situation. This project strives to have the shortest possible code base, without any abstraction. Plus it read/write the state so often, that it makes the proper capsulation worthless - essentially you end up with a bunch of copy state on the one and on the other side of the module, which certainly doesn't look good, nor making this project easier to maintain. The final goal of this refactoring is to make the request instance configurable, but before we start to think about that, we actually need that code out in modules, and right now I'm seeing this as middlewares that modifies the state of the request object, because essentially this is what happens. So again - that's not the final look of the code base, after all we hope it will live longer :) |
|
@simov I didn't mean to offend you, I'm sorry if I did. I'm not trying to minimize the size of the problem you're tackling here, or the amount of effort it requires. I just want to make sure we all understand and have thought through the concerns that come with this chosen path. It sounds like you have, so my concerns are alleviated. |
|
@FredKSchott we have a common naming convention of @simov thanks again for taking this on, I'm sorry it has been sitting for so long. In your opinion where do we go from here? |
|
oh and @FredKSchott congrats! where are you going to? |
|
@FredKSchott I'm not offended at all :) I'm sure we'll continue this discussion in the next PRs to come |
Closes #1400
Closes #1406
Closes #1407
Fixes #1409
Fixes #1412
Again on top of the previous 3 PRs. @nylen I think we should release a new version after this fix goes in.