-
-
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: add MikroOrm database adapter [v5] #1211
Conversation
…stomServiceEntity in the constructor
…lastName to createUser
… and graphql-modules v1
|
@darkbasic my knowledge of graphql-modules is very limited, so I am not sure I can really help with this technical decision, options A seems like one we should avoid for this, as you said such packages should not be aware of the context. Regarding option B "Refactor the whole accounts-js library in order to be able to take advantage of the GraphQL-Modules v2 Dependency", it's not really clear what this would mean. I am not against it, but rather wondering if such change is really necessary, mikro-orm seems to work differently than all the other database adapters. |
@pradel in the meantime I've basically rearchitectured the whole accounts-js and solved all the issues :) |
Wow this sounds amazing! |
@pradel it looks like this project saw little to no activity in the past year, which for me ironically is great news because I've just found some time to pour into this and I can try one last time to get things merged without getting mad incorporating tons of new changes. Here is my plan:
What do you think about it? |
@darkbasic love it, sounds like an amazing plan! The best would be if we can create pre-releases for each step so users can try the new major beta if needed. |
Sure, that would definitely be needed because I won't be able to catch every possible bug alone. Also the architectural design could be improved as well, but we are currently held back by some limitations of graphql-modules. We can start with what's currently possible and later see if we can get the Guild involved to enhance some functionalities of graphql-modules. |
Will this update also include an update of graphql-modules? |
Yes, |
Amazing, looking forward to the new architecture! |
@pradel the new architecture is in a state where we can start discussing and thinking about branching the legacy 0.x: master...darkbasic:accounts:new-architecture Can you please send me an invite to the accounts-js discord so we can plan a video call and go through all the changes together? I cannot find it anywhere... |
@darkbasic there is no discord, there is a slack but I lost access to my account :'( if you want to create a new one and invite me feel free, or you can dm me on discord if you prefer leopradel#0606 |
@pradel I've created an accounts.js Discord server, here is the invite link: https://discord.gg/nYSyrWPPdu |
It's on master. |
This PR depends on #1189
This PR obsoletes #1003
I've ported my old MikroORM adapted from mikro-orm v3 to the almost-released v5 and from graphql-modules 0.x to v2.
The biggest difference from my previous PR is that I gave up on the idea of instantiating a new provider on each request: it's bad on a performance perspective and since graphql-modules v2 alongside with new node.js features like
AsyncLocalStorage
allow me to do otherwise I've settled for singletons with getters instead.TODO: support any database instead of just PostgreSQL.
There are a few architectural matters I'd like to discuss before finishing this and merging the graphql-modules v2 PR.
MikroORM needs to fork its Entity Manager (
em
) on each request in order for its Identity Map to work properly. There are a few ways to do so:RequestContext
express middleware (which under the hood uses theAsyncLocalStorage
to access a fork of the Entity Manager on each request). This is necessary for the REST endpoints.context
to fork the Entity Manager and access it on each request. This is necessary for GraphQL endpoints whenever the user doesn't want to use GraphQL Modules in its application.The first option, while being mandatory for REST, poses no challenges.
The second option, while being mandatory for GraphQL when the user opts out from GraphQL Modules, enforces the need to access the context from within the adapter. We have access to the context in the resolvers, but then we call the providers (
AccountsServer
,AccountsPassword
,AccountsMikroOrm
) and these don't have access to the context. There are a few ways we can gain access to the context:A) Pass it into each and every method of these providers. I don't like this one because, along with adding a lot of boilerplate,
AccountsServer
andAccountsPassword
don't need access to the context (they are protocol agnostic after all) but unfortunately they are the ones responsible to callAccountsMikroOrm
(which needs access to the context).B) Refactor the whole accounts-js library in order to be able to take advantage of the GraphQL-Modules v2 Dependency Injection and simply inject the context in the providers. In order to get DI working we need to let GraphQL Modules instantiate providers like
AccountsServer
andAccountsPassword
instead of letting the user do so manually with the new keyword when he initializes accounts-js. We need to do something similar to my old Accounts overhaul PR. It would be simpler because GraphQL Modules v2 supports circular dependencies while v0 didn't, also this time we will be able to keep providers as singletons so the main difference will be that the user will initialize accounts-js with factories instead of instantiating the providers himself. Doing so will also allow users to provideEntityManager
via a GraphQL Modules provider if they like to do so (option 3). Such provider will have to be Operation-scoped instead of being a singleton (in order to fork the Entity Manager on each request), but that would be the only one because with GraphQL Modules v2 we can access the Execution Context of Operation-scoped providers from singleton ones.C) Another way to access the context would be to run an
AsyncLocalStorage
(where we will store the context) from each and every resolver, which will allow us to retrieve the context in theAccountsMikroOrm
provider without the need to touchAccountsServer
orAccountsPassword
(we also need to do so in the context builder because that's where we resume the session). This is the solution I implemented right now because it's the one which implements the least amount of breaking changes, but I don't like it. Since we're going to do a lot of breaking changes with the GraphQL Modules v2 PR anyway I think this is the perfect timing to introduce such further changes.The third option, while optional, can only be implemented if solution C is the chosen one.
There are other things I'd like to discuss, but these will depend on the choices we will make on the previous matters thus I won't mention them now.