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

feat(mercurius): add plugins option #2538

Merged
merged 2 commits into from Feb 6, 2023
Merged

feat(mercurius): add plugins option #2538

merged 2 commits into from Feb 6, 2023

Conversation

tugascript
Copy link
Contributor

@tugascript tugascript commented Dec 5, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

There is no plugins options on the Mercurius Adapter.

What is the new behavior?

Adds a plugins option, to be precise an array of the new MercuriusPlugin interface.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

So this is an update version of an old PR, what is different:

  • Renamed MercuriusDriverPlugin to MercuriusPlugin;
  • Removed repeated files in tests;
  • Removed state from drivers (delete options.plugins);
  • Uses for of instead of a normal for loop for readability.

@smolinari
Copy link

Really glad to see this come back and it is, what I would say, a necessity for the uptake in Mercurius usage with Nest. Thanks so much from my humble self @tugascript for the effort. Can't wait to see the hooks PR too. 😀

Scott

@Tony133
Copy link
Contributor

Tony133 commented Dec 6, 2022

I hope it gets added as a feature.

If we can document this functionality in the documentation it would be perfect. 🙌

@tugascript
Copy link
Contributor Author

tugascript commented Dec 6, 2022

I hope it gets added as a feature.

If we can document this functionality in the documentation it would be perfect. 🙌

Yeah I have a PR for the docs lined up, I will create that one when this one is merged

@julestruong
Copy link

There are few plugings that behaves weirdly.
For example, fastify-raw-body does not work if it gets registered inside a custom driver.
I'm obliged to register it in the main.ts

@tugascript
Copy link
Contributor Author

There are few plugings that behaves weirdly. For example, fastify-raw-body does not work if it gets registered inside a custom driver. I'm obliged to register it in the main.ts

This is only for plugins that have to be added after mercurius on the dependency tree, normal fastify plugins, and mercurius-upload will still be loaded in the main file. I will add a warning in the docs about that

@tugascript
Copy link
Contributor Author

@kamilmysliwiec sorry to tag you, since this is my 3rd time doing this PR, is there any change you want me to do to it? Having access to Mercurius plugins is important for a higher adoption of the framework with NestJS

@kamilmysliwiec
Copy link
Member

I'll check it out asap, hopefully before EOY @tugascript

@smolinari
Copy link

👍 Please also check this PR out too, if you could @kamilmysliwiec. #2546

Scott

@0-don
Copy link

0-don commented Jan 8, 2023

Yes this is a wanted feature

@BlakeB415
Copy link

Will this be merged soon? This is a needed feature for us.

@tugascript
Copy link
Contributor Author

Just use the custom driver from my template. Note the template is for MVP purposes and is not production ready.

@0-don
Copy link

0-don commented Jan 9, 2023

Just use the custom driver from my template. Note the template is for MVP purposes and is not production ready.

I already have a full project and just need the plugin support its just a simple merge and a lot of people will be happy

@tugascript
Copy link
Contributor Author

tugascript commented Jan 9, 2023

You can use the code on this https://gist.github.com/tugascript/7c7f2df8510303bfbbd6a30d1e3bc421 it has custom drivers based on this PR. The ones on this PR are slightly different, as I minimize the use of state (no changing objectsI, and use only true and false values (no falsy values), aka stricter TypeScript rules

@XxQuickSilverZz
Copy link

WE NEED THIS!!!

@Bit-Barron
Copy link

I would like to have this aswell since mercurius works much better then apollo and plugin support is essential

@tugascript
Copy link
Contributor Author

tugascript commented Jan 28, 2023

Well this is my last tag @kamilmysliwiec, I messaged you on discord since we are friends there, but it seems you are never online.

Could you review this before next weekend? After that I may not be able to do changes till March.

I'm not asking for it to be merged right away, I just want to know if there is any change you require for it to be merged before the new version of the nest/graphql package is released just so this feature could go in with it. Plus I need to know if this is the correct branch or there is a official next, the current one seems to be inactive.

@kamilmysliwiec
Copy link
Member

PR looks good to me overall, I can't think of any immediate (required) changes off the top of my head

@tugascript
Copy link
Contributor Author

tugascript commented Jan 28, 2023

PR looks good to me overall, I can't think of any immediate (required) changes off the top of my head

Alright, cool when this gets merged, the other PR #2546 will need to be rebased to this branch as there will be conflicts with the utility functions. If in March the Apollo v4 and Mercurius v11 compatibility PRs become stalled I will try to help on them.

Copy link
Member

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

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

Overall, code looks good, tests look great and actually test the addition, I'm happy with it. Hopefully this can help the feature get merged in sooner rather than later. Thank you everyone for your patience

@kamilmysliwiec kamilmysliwiec merged commit f435852 into nestjs:master Feb 6, 2023
@kamilmysliwiec
Copy link
Member

LGTM

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

Successfully merging this pull request may close these issues.

None yet

10 participants