accounts icon indicating copy to clipboard operation
accounts copied to clipboard

feat: add MikroOrm database adapter [v5]

Open darkbasic opened this issue 2 years ago • 4 comments

This PR depends on https://github.com/accounts-js/accounts/pull/1189 This PR obsoletes https://github.com/accounts-js/accounts/pull/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:

  1. Using the RequestContext express middleware (which under the hood uses the AsyncLocalStorage to access a fork of the Entity Manager on each request). This is necessary for the REST endpoints.
  2. Using the GraphQL 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.
  3. Using an Operation-scoped global GraphQL Modules provider implemented as a factory which forks the Entity Manager on each request. That would be opt-in: whenever the user application provides it then it would be used, otherwise it would fall back to the other methods.

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 and AccountsPassword don't need access to the context (they are protocol agnostic after all) but unfortunately they are the ones responsible to call AccountsMikroOrm (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 and AccountsPassword 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 provide EntityManager 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 the AccountsMikroOrm provider without the need to touch AccountsServer or AccountsPassword (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.

darkbasic avatar Jan 28 '22 16:01 darkbasic

⚠️ No Changeset found

Latest commit: 4c985616143a7558e74aafba296b5198f73fe04f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Jan 28 '22 16:01 changeset-bot[bot]

@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 avatar Feb 05 '22 10:02 pradel

@pradel in the meantime I've basically rearchitectured the whole accounts-js and solved all the issues :) The changes are BIG but new API is so much better that I just removed accounts/boost and database-manager because they became completely unnecessary. I tried to contact you on slack because I'd like to release a new major version of the library ASAP (realistically a development version considering the time constraints) and I asked you if we could go on a call to review the new architecture together.

darkbasic avatar Feb 05 '22 19:02 darkbasic

Wow this sounds amazing! Ah yes, I don't use slack at all these days... Do you have a discord account so we can chat?

pradel avatar Feb 07 '22 08:02 pradel

@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:

  1. ~~Rebase this PR and the PR it depends on (feat: transition to graphql-modules v2) to latest master~~ DONE
  2. ~~Update the codebase to the latest version of each and every dependency including mongodb 5 and typescript 5 and make sure that tests are still passing and examples are still running~~ DONE: https://github.com/accounts-js/accounts/compare/master...darkbasic:accounts:mikro-orm-v1-rebased-2023
  3. Take a few days to study the (still to be rebased) rearchitectured accounts-js which comes with a new much better API along with the removal of accounts/boost and database-manager which became completely unnecessary, currently sitting at my temp branch. This is a bit ironic considering I'm the one who wrote it, but one year had passed and memory fades while I will have to explain the changes to you.
  4. Rebase the rearchitectured branch on top of the already updated mikro-orm-v1-rebased-2023 branch.
  5. Video conference on Discord where we can go through the changes together and decide whether or not the new architecture is good enough to be merged.
  6. Branch the current master into a 0.x branch and merge the new architecture into master (at this point I would like to be added as a maintainer to help move things forward faster).
  7. Keep cooking until it's ready for a new major release. I have a few other major changes I'd like to get incorporated (like moving the examples away from apollo-server to yoga 3 and possibly moving from pnpm to yarn 4) but those can wait until we merge the new architecture.

What do you think about it?

darkbasic avatar May 24 '23 12:05 darkbasic

@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.

pradel avatar May 25 '23 12:05 pradel

The best would be if we can create pre-releases for each step so users can try

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.

darkbasic avatar May 25 '23 13:05 darkbasic

Will this update also include an update of graphql-modules?

pradel avatar May 25 '23 13:05 pradel

Will this update also include an update of graphql-modules?

Yes, mikro-orm-v1-rebased-2023 already updates to latest graphql-modules v2 while keeping the current accounts.js overall architecture, while the temp branch re-architectures the whole accounts.js to truly take advantage of it and have a simpler and cleaner API and codebase (which could still be improved if graphql-modules v2 supported more functionalities).

darkbasic avatar May 25 '23 13:05 darkbasic

Amazing, looking forward to the new architecture!

pradel avatar May 25 '23 14:05 pradel

@pradel the new architecture is in a state where we can start discussing and thinking about branching the legacy 0.x: https://github.com/accounts-js/accounts/compare/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 avatar Jun 03 '23 10:06 darkbasic

@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 avatar Jun 05 '23 09:06 pradel

@pradel I've created an accounts.js Discord server, here is the invite link: https://discord.gg/nYSyrWPPdu

darkbasic avatar Jun 06 '23 10:06 darkbasic

It's on master.

darkbasic avatar Sep 13 '23 08:09 darkbasic