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

Add rewrite_response #209

Merged
merged 6 commits into from
Nov 1, 2021
Merged

Add rewrite_response #209

merged 6 commits into from
Nov 1, 2021

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Aug 2, 2020

Hey all, here's what I came up with to resolve #202.

I seem to have achieved my original goal, but there's quite a lot I'm dissatisfied about.

  1. In order to be able to read the config in the appropriate spots, I ended up having to change the signatures of several functions. Will this cause breakage in other projects? Is there some better way which I didn't see?
  2. I couldn't make my new rewrite_response function take just response as input without losing important information. Thus I modified my original specification, and rewrite_response is now a function of host, port, path, and response.
  3. I had to implement a near-duplicate function non_service_rewrite_response to handle the /proxy/ endpoint. In this case I'm skeptical of the jupyter-server-proxy design. Namely, I would find it more intuitive to first define a correspondence between "j-s-p server" objects and endpoints, and then implement the /proxy/ endpoint as a "j-s-p server". Was it a conscious design decision not to do this, or is it just the way things evolved?
  4. I would like to be able to intercept redirects, but if I'm not mistaken, redirects typically have no response body. Thus my rewrite_response won't be of any help here.
  5. I couldn't figure out how to rebuild the docs.

I do think this PR is a step forward. Regarding the points above, would it make sense to address any quick fixes, merge, and then make issues for any outstanding points?

@ryanlovett
Copy link
Collaborator

Hi @maresb ,

First, I'm very sorry for the delay, but thank you very much for the PR including the extra cleanups. Regarding your questions:

  1. Possibly? Most downstream projects just use traitlets rather than subclassing. I don't know of a project which heavily relies on the latter. If we were using semver we could bump the major version to signify an API change, however we use the major version to reflect Lab compatibility. We could bump the minor version and mention breakage in the README or some such.
  2. Things evolved this way. Originally there were no named servers.
  3. We do have request_headers_override but not response_headers_override.

How would you feel about rebasing on master? I'd be happy to merge.

@maresb
Copy link
Contributor Author

maresb commented Oct 30, 2021

@ryanlovett, wow, it's been a while since I thought about this! 😆 I'm pleasantly surprised that you're interested in merging. I just rebased onto master.

Before rebase: 6e5313a
After rebase: ca0c5fd

Probably before merging I should have one more look over my commits and make sure they still do what was intended, and that I didn't make any errors during the rebase.

@maresb
Copy link
Contributor Author

maresb commented Oct 30, 2021

BTW, what do you think about black/pre-commit? It may be nice to standardize the formatting, but then that messes with all the outstanding PRs. But then maybe one could use something like this to rebase existing PRs.

@ryanlovett
Copy link
Collaborator

Thanks @maresb , I really appreciate the rebase! Just let me know when you think it is ready.

I wouldn't mind having some sort of automatic formatting. Not having to make formatting decisions appeals to me. That can probably be proposed in a new issue and then other project members can chime in.

@maresb
Copy link
Contributor Author

maresb commented Oct 31, 2021

@ryanlovett I'm fairly convinced that it's ready now. (I was worried that if I made a mistake with the rebase then I might inadvertently revert something.)

If it looks good to you, then please merge.

@ryanlovett ryanlovett merged commit 5fd3969 into jupyterhub:master Nov 1, 2021
@ryanlovett
Copy link
Collaborator

Thank you again @maresb !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inject a <script> tag into each HTML page
3 participants