Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

lazy mode and deferred connection params #124

Merged
merged 7 commits into from
May 17, 2017

Conversation

thebigredgeek
Copy link
Contributor

@thebigredgeek thebigredgeek commented May 10, 2017

TODO:

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change

@thebigredgeek
Copy link
Contributor Author

Will write tests once the design is approved.

@thebigredgeek
Copy link
Contributor Author

thebigredgeek commented May 10, 2017

seen here: #123

should solve dynamic authenticate needs

@thebigredgeek
Copy link
Contributor Author

bump @helfer @Urigo @stubailo

@helfer
Copy link
Contributor

helfer commented May 12, 2017

@thebigredgeek I think @Urigo (busy atm) or @NeoPhi should review this. Reviewing will be a lot easier and PRs will be merged faster if you make this two separate (maybe stacked) PRs, one for the connectionParams function, one for the lazy connecting. I think connectionParams function is pretty easy to test and merge, lazy connecting might require more testing and more discussions.

@thebigredgeek
Copy link
Contributor Author

I can separate them, but merging one but not the other doesn't help me unfortunately. I'll try to find time for that if it ends up being necessary

@Urigo
Copy link
Contributor

Urigo commented May 14, 2017

@thebigredgeek great job.
I like both the connectionParams and the lazy connecting.
Few comments:

  1. I don't see a reason to explicitly convert to Boolean. we never use it so can we remove that?
  2. Rebase against the latest master and make sure it work with the new API introduced here
  3. @srtucker22 I think this touches on the same problem that you solved on your PR. Your PR though creates a more robust approach with middlewares. do you think we should introduce both solutions or just use one of those approaches for simplicity of the API? also @thebigredgeek let me know your thoughts on that.

@srtucker22
Copy link
Contributor

srtucker22 commented May 14, 2017

I personally like both approaches :)
@thebigredgeek made a good point that he preferred to only have authenticated WS connections to avoid needing to build in a lot of DOS security. There are also occasions where ppl will require an open connection without auth ala Meteor and would prefer to just keep the connection open instead of disconnect/reconnect when auth changes. They'll just have to employ some extra security (as you need to do with meteor) to secure against DOS when ppl load up on unauthed connections.

I also think there is a place for middleware before a subscription for other use cases to manipulate context or other features of the subscription request.

So I think both pieces are useful separately and together. That's just my $.02

@thebigredgeek
Copy link
Contributor Author

thebigredgeek commented May 15, 2017

Yeah I agree @srtucker22 . I think the middleware stuff is super useful, but I want to be able to reject connections as they occur rather than just relying on ACL to determine whether a user should be able to subscribe etc. My main use case here is react native... where I can't do a "full page refresh" after login (which in my web app, login state is used to determine whether or not to decorate with the subscription transport network interface).

@thebigredgeek
Copy link
Contributor Author

@Urigo

  1. Will do, I just tend to code more defensively with respect boolean stuff due to PTSD from earlier versions of JS. Old habits die hard, but should probably die at this point ;)
  2. Yup, will do in the AM :)

@thebigredgeek
Copy link
Contributor Author

Also @Urigo how do we want to test this? Seems like it might break a lot of the existing tests

@Urigo
Copy link
Contributor

Urigo commented May 16, 2017

@thebigredgeek can you please explain why it might break the existing tests? the default behavior should be with lazy flag set to false, and the connectionParams as regular object, so the existing tests should be fine.

@Urigo
Copy link
Contributor

Urigo commented May 16, 2017

@thebigredgeek I've also merged @srtucker22 changes, so please rebase again

@Urigo Urigo mentioned this pull request May 16, 2017
5 tasks
@thebigredgeek
Copy link
Contributor Author

@Urigo ah yeah true. I guess the defaults to existing behavior means no breaking tests :D. Cool, I'll rebase and add test coverage.

@Urigo Urigo merged commit f603def into apollographql:master May 17, 2017
@Urigo
Copy link
Contributor

Urigo commented May 17, 2017

thank you very much @thebigredgeek !

@thebigredgeek thebigredgeek deleted the feature/lazyConnection branch May 17, 2017 21:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants