-
-
Notifications
You must be signed in to change notification settings - Fork 142
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: transition to graphql-modules v2 #1189
Conversation
|
Not sure why tests are failing on CI, they are working on my machine:
Might be worth trying to re-run the CI. |
Codecov Report
@@ Coverage Diff @@
## master #1189 +/- ##
==========================================
+ Coverage 95.29% 96.05% +0.75%
==========================================
Files 106 96 -10
Lines 2381 2305 -76
Branches 492 480 -12
==========================================
- Hits 2269 2214 -55
+ Misses 103 85 -18
+ Partials 9 6 -3
Continue to review full report at Codecov.
|
Thank you so much for the pr @darkbasic, I will be able to have a look only next week 🙏 |
@darkbasic I started to review the pr, but the amount of unrelated changes makes it really hard :(
|
@darkbasic I upgraded the deps to limit the number of changes in this pr.
This should be solved by upgrading the deps right? In general, pr looks good, I am just wondering if we could somehow find ways to reduce the boilerplate to create a new Graphql server. Other than this, updating the docs and creating a migration guide will be needed with the new release, amazing job 🚀 |
I see that you've backported most of the fixes/updates into the main branch, thus I've rebased this PR against it. |
AFAIK this issue should have been solved in graphql-modules 2.0.0, but I still didn't have the time to test it and thus I didn't bump the graphql-modules version. Hopefully I should be able to do so in the next days. |
Ok I've rebased this PR once again and bumped graphql-modules to 2.0.0, which fixes the issue and doesn't require to override its @graphql-tools/wrap version. |
Hi, thanks for a great project! I'm running in to issues trying to set up a new project from scratch by following the guides. Issues like being unable to use the old declarations by default, since they've been removed from graphql tools, and Apollo 3 uses a newer version. It's possible to work around the issues that I've found by using older versions of graphql tools, but it wasn't especially easy to figure that out. It might be nice to get those notes in to the older docs, if they are going to stick around after this releases. I'll be happy to test this PR out in my new project, since I don't have any interest in using older modules. I'll give it a try this weekend. Seems like I'll have plenty of time with the coming blizzard! |
@darkbasic I remember an old game engine called "Dark Basic". That brings me back! |
@CaptainN that's exactly where my nickname comes from: I used it to develop some games with it when I was a kid :) By the way your timing is very precise: I'm planning to do further breaking changes here and I've just discussed them in my other MikroORM v5 PR. You're welcome to join the discussion. |
Awesome! Maybe I'll make a PR for Prisma (based on this adapter), which is what I'm using. Would there be interest in having that adapter in the repo? |
Sure if you're willing to maintain it, because I personally don't use Prisma. But I suggest you to wait a little bit until the waters settle down before PRing: there are a lot of breaking changes down the pipe right now. |
Is it known when a version including this update is going to be released? |
@darkbasic I am still willing to merge this one, happy to chat on Twitter or Discord! |
Hi guys, I am pretty excited about this change. Any updates on that? it would be amazing to have it running in Apollo V3 Thanks! |
Happy to help get this merged, if needed. I'm thinking it might solve this issue #1238 for me. |
@darkbasic since you originally did this work almost a year ago (in October 2021) does it make sense to version bump again, or are we past that point? |
Hi, |
I hear that!
Let me know if there’s anything I can do! I’d happily roll your changes into my project to help test too, like I said, anything I can do to help roll this out. All of what you said sounds really good. I was actually a week or two away from either writing my own GraphQL API—because of out-of-date dependencies, the @auth directive issues I linked and security audit issues—or forking it and doing my own updates. |
Testing would be appreciated of course, once @pradel gets a chance to review the changes I would like to release an alpha and testing would be needed to ensure that there are no regressions.
Do you have a link to your audit? |
Here's the parseable version, and here's the JSON version.
Then a critical and moderate vulnerability comes from The last one is Mercurius which I can't update that package yet because it causes the following chain reaction that ends with
But in order to do 5, I have to upgrade Hope that helps. I didn't get a chance to proof read because I'm in a hurry, so hopefully that all makes sense. Let me know if there are any questions. |
Ah you meant the npm thing, I thought someone did a real security audit on accounts-js. |
Hey @darkbasic, it's been a long time I didn't check the changes, I don't really have time now but I can release the change made on this pr as an alpha tag, would that work for you in order to test? Btw if you want to chat I am on discord |
Hi @pradel, there is no reason to release an alpha because the code I'd like to get merged is on a completely different branch and I didn't open a PR yet. I'd like to go through the code in a video call to show you the new architecture. I'll ping you on discord once I'm ready. |
@darkbasic it would be beneficial to the community to have a somewhat similar tour around the architecture, in the form of additional documentation, as part of this release. Perhaps as a “what is accountsJS” or a Getting started section about which pieces to click together? Have you already put together something like that? I could contribute in that effort, perhaps a similar tour after @pradel approved things or they get more finalized, or you could record the tour on video and we can use it in the docs. I think all the changes sound really good. I was just thinking about how the docs are pretty terse, and could use some additional details. Seems like a good opportunity. |
I have rewritten all the examples and I plan to make a blog post when it will be time for a release, but the documentation still needs to be updated. If you want to do so you are more than welcome! |
Hi, I've overhauled the whole accounts.js to a brand new architecture which better suits graphql-modules while making it simpler and more straightforward. I've also created a brand new MikroORM adapter and updated all the dependencies including @graphql-tools 10, graphql 16, graphql-modules 2, @apollo/server 4, typescript 5, mongo 5 etc. In accordance with @leopradel I've opened a new Discord server because he lost access to his Slack account, here is the invite link: https://discord.gg/nYSyrWPPdu |
It's on master. |
This PR doesn't simply port accounts-js to graphql-modules v1 but also updates every dep, including latest graphql-tools. That means I've basically rewritten some examples from scratch (like schema stitching).
I tested it and it seems to work fine, also tests are passing.
Unfortunately there is a bug in graphql-modules which breaks schema stitching (Urigo/graphql-modules#1914) and at the moment there isn't any working version on the horizon.
I circumnvented the issue by overriding the bundled graphql-tools version (https://github.com/darkbasic/accounts/blob/f078633c03e9a432db66fc8a507db7d207ef1e97/package.json#L75) and it works fine now, but I'm unsure if there might be any undefined behaviour because of that (couldn't find any).
Should hopefully be fixed in a future graphql-modules release.
P.S. I didn't test the docusaurus because I've seen there was already a pending PR for that which updated all of its dependencies anyway.