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 configuring multiple routes that point to different endpoints #14

Merged
merged 3 commits into from
Apr 13, 2023

Conversation

mfn
Copy link
Contributor

@mfn mfn commented Apr 12, 2023

Summary

Instead of a single route definition, multiple routes can be defined.

They "key" is the URI name and necessary for the controller to look up the config for e.g. the endpoint.

The diff is pretty wild, it's best viewed with disabling whitespace.

This is obviously a major breakage and would require a new major version 😬

Notes

Before making the PR I verified the feasibility by modifying vendor files in a private project using rebing/graphql-laravel to achieve the desired effect.

TODO

  • Fully update README if we agree on the implementation
    Some parts are already outdated but in general a few more pieces should be adjusted.

Links

@mfn mfn force-pushed the mfn-multi-route branch 2 times, most recently from c290104 to a61f741 Compare April 12, 2023 19:01
@mfn mfn marked this pull request as ready for review April 12, 2023 19:02
Copy link
Contributor Author

@mfn mfn left a comment

Choose a reason for hiding this comment

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

@spawnia what do you think about this approach?

src/GraphiQLController.php Outdated Show resolved Hide resolved
src/routes.php Outdated Show resolved Hide resolved
Copy link
Member

@spawnia spawnia left a comment

Choose a reason for hiding this comment

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

I think this is the way to go, nice work

src/GraphiQLController.php Outdated Show resolved Hide resolved
src/routes.php Outdated Show resolved Hide resolved
src/routes.php Outdated Show resolved Hide resolved
views/index.blade.php Show resolved Hide resolved
@spawnia spawnia added the enhancement New feature or request label Apr 13, 2023
@mfn mfn force-pushed the mfn-multi-route branch 2 times, most recently from 50a38c2 to c92c810 Compare April 13, 2023 08:27
Copy link
Contributor Author

@mfn mfn left a comment

Choose a reason for hiding this comment

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

All feedback addressed AFAICS!

Copy link
Member

@spawnia spawnia left a comment

Choose a reason for hiding this comment

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

The tweaks are getting smaller 😉

README.md Outdated Show resolved Hide resolved
src/GraphiQLController.php Outdated Show resolved Hide resolved
src/routes.php Show resolved Hide resolved
Instead of a single route definition, multiple routes can be defined.

They "key" is the URI name and necessary for the controller to look up
the config for e.g. the endpoint.
@mfn mfn force-pushed the mfn-multi-route branch from c92c810 to e8791c4 Compare April 13, 2023 10:33
@spawnia spawnia changed the title Introduce multi-route support Allow configuring multiple routes that point to different endpoints Apr 13, 2023
@spawnia spawnia merged commit 336ecbc into mll-lab:master Apr 13, 2023
@mfn mfn deleted the mfn-multi-route branch April 14, 2023 05:51
@mfn
Copy link
Contributor Author

mfn commented Apr 14, 2023

Thanks for merging and also putting in some final touches! Already tried in a private project, all working so far 🙏🏼

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

Successfully merging this pull request may close these issues.

Allow configuring multiple routes that point to different endpoints
2 participants