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

Support playgroundPath #1974

Closed
wants to merge 6 commits into from
Closed

Conversation

mondaychen
Copy link
Contributor

@mondaychen mondaychen commented Nov 15, 2018

This is to implement issue #1908

Please advise me on where I should add docs for this new feature. Thanks!

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • 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

@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Nov 15, 2018
@avocadoslab
Copy link
Contributor

This one is huge! Thanks @mondaychen for knocking this out. I will take a look at this soon.

Copy link
Contributor

@avocadoslab avocadoslab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did check express changes and they look good to me.

@jole78
Copy link

jole78 commented Dec 5, 2018

What's happening with this? It would be a huge win for us to have this since we need a way to "fake" a jwt token header when a developer is using the playground and we can't just turn off auth all together.
This worked fine with yoga but we're now switching to pure apollo and really missing this feature.
With yoga we could just check if the incoming referrer was the playground url (which was then different) and the NODE_ENV was development as just add a valid user to req.user during context creation.

@mondaychen
Copy link
Contributor Author

hey @abernix can you say something about what are you going to do with this PR? I'm pretty sure you do read PRs diligently, but I have to say I feel terrible when my own PRs are open for almost a month with no comment from any maintainer.

I understand this is not working on features or bugs with the highest priority, but I, along with several others, do believe this is a good feature to bring in and will use it immediately when a new version comes with this feature.

Let me know if you

  1. think this feature is not worth implementing
  2. think this PR is low quality
  3. want to get something else merged before this
  4. need more time before you can really take a look at it

@eric-burel
Copy link

Hi, to answer point 1, I am upgrading Vulcan.js to Apollo 2 and this feature sounds necessary to us. Having Playground on the same endpoint as the /graphql API, though an original idea, sounds confusing/weird/a potential source of unexpected technical issue. That's almost the first thing that was mentioned during code review.

@FanAs
Copy link

FanAs commented Feb 28, 2019

Hi.
Any chances this will get finally merged? I've been following this ticket for a long time now and I'm really dissapointed.

@abernix
Copy link
Member

abernix commented Mar 4, 2019

@mondaychen Starting with your questions:

  1. think this feature is not worth implementing

This is presently my feeling, yes, but I'll get into details below and perhaps I’ll feel differently after we discuss the feature some more.

  1. think this PR is low quality

Not at all! In fact, this appears to be of great quality! Thank you for submitting a high quality PR with tests, changes to the most popular integrations, CHANGELOGs, etc. I'm sorry if the delay in response led you to doubt the quality of your work!

  1. want to get something else merged before this

Well, yes, but that's okay. There's a lot of PRs I'd like to get merged! There's generally just some concerns which are holding them up, and I'm hoping we can get better at communicating about those (and I'll try to explain how here!)

  1. need more time before you can really take a look at it

I did, but this is mostly the same as 3! I'm really sorry that it's taken so long to get back to you, I'm cognizant that much of the frustration (of many individuals above!) is likely due to the time.

Ok.. that said, I realize you took some time to work on this, but to be relatively direct to everyone asking what the hold up is: you opened your own PR, for your own issue, after declaring it as a valid feature yourself, without consensus from the maintainers of the project. I'm not saying "Don't do that", but I'm afraid it's the reason there's some disappointment being voiced here by yourself and others.

The requirements for submitting requests for new features are explained in the contributing guidelines. Those guidelines also specify to wait for consensus before opening a PR, especially for PRs which are more than ~20 lines.

I hope you understand that every feature has a cost and we should be somewhat selective about what's accepted. Having concrete use-cases is a great place to start, and the original issue you opened didn't have that. This PR adds 200+ lines of code, so it's certainly not a trivial investment for the project and its my duty to try and look out for that, even if it comes at some discomfort.

Anyhow, I hope you'll understand a number of my hesitation, particularly because I think there are other ways to do this and while flexibility is important, more flexibility often comes with more configuration and complexity.

You did recently provide a use-case in the original issue:

In my case, I need to protect my graphql API with authentication rules via some nginx/gateway configuration. But for the playground in development mode, I want to get rid of them.

I have some questions about this, namely:

  • What type of authentication?
  • Is Nginx checking for a header?
  • Could you check for a different header?
  • Does the same proxy not support development vs production? (Or is it the same proxy during development and production?)

@jole78:

It would be a huge win for us to have this since we need a way to "fake" a jwt token header when a developer is using the playground
With yoga we could just check if the incoming referrer was the playground url

Ignoring the fact that the Referer header should not be used for auth purposes in most cases, why can't you still check Referer? Or maybe Accept (which is what Apollo Server uses when deciding to render playground itself!)?

Referer is a header introduced by the client (browser, in this case); if Referer is present and its https://hostname/graphql (or whatever the path is for the GraphQL endpoint), you should be able to be as confident that it came from GraphQL Playground as you were before, no?


@eric-burel

this feature sounds necessary to us.
Having Playground on the same endpoint as the /graphql API, though an original idea, sounds confusing/weird/a potential source of unexpected technical issue

I don't think we can fix technical issues just because they're too original. 😄 Can you help me better understand the concerns to this approach that were brought up during the code review?


Realistically, my temptation is to close this PR and take this to the design table in the original issue where I suspect we might actually find a reasonable workaround that doesn't bring this additional surface area, but we can hold off on that.

I also have an inkling that if we can do a better job at the Improve integration with existing frameworks bullet-point in #2360, this might be as simple disabling Playground in Apollo Server (already possible with playground: false) along with the more traditional:

import { graphqlPlayground } from 'possibly-even-apollo-server-still';
someHttpFramework.use('/graphql-playground', graphqlPlayground({ service }));

Anyhow, looking forward to improving this experience! Feedback appreciated.

@jole78
Copy link

jole78 commented Mar 5, 2019

@abernix Thanks for the response. Your detailed response is highly appreciated even though some might not agree. Now we at least know the reasons.
I have been able to work around my issues with playground using this approach (which may or may not be a correct one but anyway):

I figured out that when configuring apollo server we can tell the playground what endpoint it's gonna use, like this:

  const server = new ApolloServer({
    schema: schema,
    context: ({ req }) => createContext(req),
    uploads: false,
    playground: {
      endpoint: "/graphql/dev",
    },
  });

...then when we setup the context....

function createContext(request) {
  if (isDevelopment()) {
    if (request && request.originalUrl.endsWith("/graphql/dev")) {
      // fake a user on the request...
      request.user = {
        name: "GRAPHQL DEVELOPMENT USER",
        permissions: [SOME PERMISSIONS NEEDED],
        roles: [SOME ROLES NEEDED],
      };
    }
  }
....

...we can just look for that endpoint being playground and then "fake" a request.user object...

So, that's one option at least...and I can live with that ;) Keep up the good work you guys do!

@eric-burel
Copy link

@abernix Thanks for this detailed response! To provide more info, what bothers me is the extra overhead due to the fact that the endpoint is /graphql.
It is hard for me to give more detailed examples than those already posted here, because that's my point: we can not fully predict how the /graphql endpoint is used by developers.

I would reverse the question: are you sure that adding playground systematically to the /graphql endpoint will never raise issues, in particular tied to devops, development environment, and creating specific features?

It seems to at least complicate dev workflows.
I know it will sound silly but it also slowed me down during debug. In Vulcan, we had weird behavior due to a patch we used for meteor/webapp. We could not help but wonder if it was related to Playground. In the end it wasn't, but still, having a 3rd party tool tied to /graphql makes me uncomfortable, when I have a bug I can't help but wonder if it's related and thus I lose time exploring a wrong track.

Also we can imagine other use cases: say that I want to tie an interface to the /graphql endpoint that is not Playground? For example a live documentation of my API for 3rd parties (pretty sure you'll see this use case sooner or later), another development tool that I feel more relevant at the moment, etc.

Note that I really like the syntax you provided here:

import { graphqlPlayground } from 'possibly-even-apollo-server-still';
someHttpFramework.use('/graphql-playground', graphqlPlayground({ service }));

That would make Playground more consistent with how other libs are working and would totally solve this issue for me.

@mondaychen
Copy link
Contributor Author

@abernix I really appreciated your comment. In recent a few months, I've learned a lot about the workload and pressure that maintainers of a popular project are facing. Please accept my apologies for being too pushy and not waiting for a consensus.

To answer your question:

  • In our system, we have a gateway service that checks header and cookie/session to make sure the requests to our API endpoints are from valid sources by valid users.
  • It does check for a header
  • Since the header has been used everywhere for a while, I don't think I can convince people to change it only for a dev tool
  • It's used for both development and production. It's a consensus in our team that we should avoid dev-only or prd-only set-up, as long as it's possible. If this API works perfectly locally, but fails on production, it would be hard to debug.

@mattcan
Copy link

mattcan commented Apr 26, 2019

Our team is in a similar situation as @mondaychen where we would prefer not to disable our gateway checks in the development environment.

@mchong-teal
Copy link

Also would love to have this feature.

@yukw777
Copy link

yukw777 commented May 16, 2019

I'd love this feature also. I have a similar set up as @mondaychen.

@thardy
Copy link

thardy commented Sep 5, 2019

We have need of this functionality as well. To explain our specific need:

  • The playground page needs to be secured behind a membership login (using a simple session login)
  • The api needs to be secured behind completely different authentication (a header token generated and controlled by external developers - like how Google api allows developers to generate a token and whitelist the referrers that can use that token)

We need these endpoints to be completely separated and easily managed, separately. Having them bundled into a single endpoint separated only by an HTTP verb is making this difficult for us.

@etaylor23
Copy link

We also require this functionality. Specifically, we would like to offer 2 URLs - one for the playground and one for a JSON representation.

This was previously possible in a Hapi/GraphQL set up where we could manually register 2 distinct routes. No longer possible now we're using Apollo

@sripberger
Copy link

sripberger commented Dec 10, 2019

I can't say I understand how this could be described as something without a "valid use case."

A rather obvious problem introduced by serving the playground on the same path as the GraphQL API itself is that it causes GraphQL GET requests (which are otherwise supported by apollo-server, as well they should be) to be swallowed by the middlewares serving the playground.

So, even if I don't have gateway checks or other authentication middlewares that I need to bypass when serving the playground-- like others have mentioned-- I still can't use GET requests to my development instance without disabling the playground.

To be perfectly honest, I'd consider this a fundamental design error-- something that probably should have been considered when adding the playground in the first place. The whole reason you put a graphql endpoint at its own path like /graphql is so it doesn't conflict with other things being served at the same host. Things like, say, web interfaces.

I realize that we can turn off the automatically-added playground middlewares with playground: false and add the playground middlewares at the desired paths ourselves. But having an easier way to handle this without (in Koa's case) futzing around with koa-router would be nice.

@abernix abernix closed this Jun 24, 2020
@abernix abernix reopened this Jun 25, 2020
@abernix abernix changed the base branch from master to main June 25, 2020 09:05
@DarkLite1
Copy link

I stumbled upon this PR looking for a solution on how to get the Playground URL different from the Graphql URL. It would allow us to bypass the passport authentication mechanism for Playground when being in development mode. Eagerly waiting fro this feature...

@glasser
Copy link
Member

glasser commented Oct 4, 2022

GraphQL Playground's maintainers have retired the project. It is not the default experience as of last year's Apollo Server 3 and won't be distributed as a maintained part of Apollo Server starting with Apollo Server 4 (imminent). So we are not going to improve it. Sorry for the delay!

@glasser glasser closed this Oct 4, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⛲️ feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet