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

Allow GraphQL endpoint to be conditionally included. Closes #209 #210

Conversation

zachfeldman
Copy link
Contributor

@zachfeldman zachfeldman commented Aug 21, 2023

Summary

Adds a Rails configuration option to include /graphql only if it is true.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

@zachfeldman zachfeldman force-pushed the 209-conditionally-include-graphql-route branch from 2e3151c to e744029 Compare August 21, 2023 18:36
@zachfeldman
Copy link
Contributor Author

@waiting-for-dev could you take a glance at this when you get a chance? Thank you!

@waiting-for-dev
Copy link
Contributor

Hey @zachfeldman, many thanks for sending this change request. I'd like to understand the context here better. Given that the endpoint will only be made available when you install the extension into your project, in which situation would you like to turn it off? Is the idea to have the configuration act like a feature flag? I can think of two approaches that don't involve moving the feature flag upstream:

  • You can have gem "solidus_graphql_api", require: false in your Gemfile. Probably not helpful as it doesn't happen at runtime, but as I don't know your use case yet, maybe it's worth pointing out.
  • As we're doing with the new Stripe extension or the new admin, instead of monkey-patching the core routes, we can have the route defined in SolidusGraphqlApi::Engine and write the mount line to the application's config/route when running bin/rails g solidus_graphql_api:install. We can say that's our new recommendation for extensions, and that would allow you to change the code in any way you need.

What do you think?

@zachfeldman
Copy link
Contributor Author

Hey @waiting-for-dev thanks for the fast response here!

The use case is that we're testing GraphQL in development, but also in our Review Apps and Staging env on Heroku that are "production" environments. So just putting them in a specific group in the Gemfile doesn't really cut it.

We don't want to turn it on in prod yet because we haven't analyzed the security implications of it, nor is our new front end ready for prime time quite yet.

I think option 2 would work really well, and I know that's how you're doing it with the new admin. If you are down with that based on the above, I can change my PR to do this instead and also edit the README!

@waiting-for-dev
Copy link
Contributor

I think option 2 would work really well, and I know that's how you're doing it with the new admin. If you are down with that based on the above, I can change my PR to do this instead and also edit the README!

That would be awesome, @zachfeldman. Looking forward to it. Please, let me know if you need any help.

@zachfeldman zachfeldman force-pushed the 209-conditionally-include-graphql-route branch from e744029 to 0d8dcb8 Compare August 22, 2023 23:59
@zachfeldman
Copy link
Contributor Author

@waiting-for-dev I copied from the main Solidus repo and this seems to work locally for me when using this branch and running the generator - how's that?

@waiting-for-dev
Copy link
Contributor

Hey, @zachfeldman, it looks like the GraphQL controller is not found. That's probably because we're not mounting the engine into the host application. We need to inject something like the following in the host routes:

mount SolidusGraphqlApi::Engine, at: '/graphql'

And define the route in the engine config/routes.rb file.

I tried locally, but still it didn't work. I run out of time today, but I'll try again tomorrow unless you're quicker than me 🙂

@zachfeldman zachfeldman force-pushed the 209-conditionally-include-graphql-route branch from 0d8dcb8 to 648ccfb Compare August 23, 2023 16:02
@zachfeldman
Copy link
Contributor Author

@waiting-for-dev this commit seems to do it right for me! LMK if you have any opinions about the URL structure or the class name for routing, glad to adjust.

@zachfeldman zachfeldman force-pushed the 209-conditionally-include-graphql-route branch from 648ccfb to 13bb9dd Compare August 24, 2023 14:25
@zachfeldman
Copy link
Contributor Author

@waiting-for-dev updated!

Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this, @zachfeldman! ❤️

@waiting-for-dev waiting-for-dev merged commit 3370ef4 into solidusio:main Aug 25, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants