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.

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

Fix basic auth #1413

Merged
merged 7 commits into from Feb 11, 2015

Conversation

Projects
None yet
3 participants
@simov
Copy link
Member

commented Feb 7, 2015

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.

@simov

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2015

@nylen I think only the last commit here needs review, since @seanstrom reviewed the rest of the commits.

FredKSchott referenced this pull request Feb 9, 2015

FredKSchott FredKSchott
Move _buildRequest() logic back into init
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.
@FredKSchott

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2015

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

@simov

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2015

Think about these 4 request modules as Express middlewares, where res.locals is called self.request in our case. Plus that may change, but we can't just magically switch from code base written in one file to something perfect, unless we start a new project from scratch probably. The refactoring should be made over the time, and since it was not, we'll need some time to get there.

@simov

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2015

Anyone I think we should merge this soon, @nylen ?

@FredKSchott

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2015

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.

@simov

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2015

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

@FredKSchott

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2015

@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.

@nylen

This comment has been minimized.

Copy link
Member

commented Feb 11, 2015

@FredKSchott we have a common naming convention of onRequest and onResponse now, so IMO the next step is to register everyone that wants to use these hooks, then call them all in the same place. I think that helps address the state concerns, thoughts?

@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?

nylen added a commit that referenced this pull request Feb 11, 2015

@nylen nylen merged commit 2f5f5e1 into request:master Feb 11, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@nylen

This comment has been minimized.

Copy link
Member

commented Feb 11, 2015

oh and @FredKSchott congrats! where are you going to?

@simov

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2015

@FredKSchott I'm not offended at all :) I'm sure we'll continue this discussion in the next PRs to come

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.