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

refactor: zitadel server #63

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yordis
Copy link
Contributor

@yordis yordis commented Apr 7, 2024

Context

Primary

Refactor the @zitadel/server to remove singletons and extra indirection around initializing the clients. Depending on the users ' needs, application-level code decides whether to use single-client or multi-client code.

Under apps/login, I simplify the clients' usage. Since the functions are defined in the same file (or could be imported directly), the code does not need to be passed around; UNLESS multi-client is required.

Also, some abstractions add extra indirections around the SDK API with little value. Since the SDK API already has structured messages, I removed most of them, except the ones that shared some "Message Structure Logic."

Since there are collisions between protobuf services, we need to expose the client around the protobuf service, for example, SearchQuery from session and user; we can not hide the layout of the services!

Extras

  • Changed alias from #/ to @/, just for familiarity
  • Upgraded dependencies
  • Ran prettier

Definition of Ready

  • I am happy with the code
  • Short description of the feature/issue is added in the pr description
  • PR is linked to the corresponding user story
  • Acceptance criteria are met
  • All open todos and follow ups are defined in a new ticket and justified
  • Deviations from the acceptance criteria and design are agreed with the PO and documented.
  • Jest unit tests ensure that components produce expected outputs on different inputs.
  • Cypress integration tests ensure that login app pages work as expected. The ZITADEL API is mocked.
  • No debug or dead code
  • My code has no repetitions

Copy link

vercel bot commented Apr 7, 2024

@yordis is attempting to deploy a commit to the zitadel Team on Vercel.

A member of the Team first needs to authorize it.

@yordis yordis force-pushed the refactor-zitadel-server branch 3 times, most recently from 967e834 to 7b7c28a Compare April 7, 2024 09:01
@muhlemmer muhlemmer added the devx label Apr 8, 2024
Copy link

vercel bot commented Apr 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
typescript-login ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 23, 2024 8:31am

@peintnermax
Copy link
Member

Hey @yordis thx for this amazing PR. When compiling we have a couple of issues.
when using 'node' as module resolution in the application, type declarations cannot be used as they are exported.
Do you have a suggestion on how to fix this?

packages/zitadel-server/src/v3alpha.ts Outdated Show resolved Hide resolved
packages/zitadel-server/src/v3alpha.ts Outdated Show resolved Hide resolved
@yordis yordis force-pushed the refactor-zitadel-server branch 7 times, most recently from a1b349e to 7219bc3 Compare April 13, 2024 02:18
@yordis
Copy link
Contributor Author

yordis commented Apr 13, 2024

@peintnermax sorry for the whole mess, I am trying to upgrade deps and get the entire setup to compile properly.

I got the basic refactoring to the server, but the DevOps and whatnot are giving me trouble, and I got too deep into upgrading things by now ... Hopefully it is worth it

@yordis
Copy link
Contributor Author

yordis commented Apr 13, 2024

CJS ⚡️ Build success in 2201ms
src/proto/server/zitadel/admin.ts(31032,62): error TS2345: Argument of type 'import("/Users/ubi/Developer/github.com/zitadel/typescript/packages/zitadel-server/src/proto/server/zitadel/org").Domain' is not assignable to parameter of type 'import("/Users/ubi/Developer/github.com/zitadel/typescript/packages/zitadel-server/src/proto/server/zitadel/instance").Domain'.
Type 'Domain' is missing the following properties from type 'Domain': domain, primary, generated

@peintnermax is there anyway to ping to a tag version the buf generate repo? I am 5000% sure that @zitadel/server was building correctly, but now it is not, the error is coming from inside the proto which is even more concerning since it is part of the codegen

@yordis yordis force-pushed the refactor-zitadel-server branch 3 times, most recently from 43b69a7 to c9967e0 Compare April 14, 2024 23:11
@peintnermax
Copy link
Member

@peintnermax sorry for the whole mess, I am trying to upgrade deps and get the entire setup to compile properly.

I got the basic refactoring to the server, but the DevOps and whatnot are giving me trouble, and I got too deep into upgrading things by now ... Hopefully it is worth it

Any help is greatly appreciated.. Thx for your effort 😎👍
We can generate the stubs by specifying the tag like this buf generate https://github.com/zitadel/zitadel.git#tag=v2.49.3

@yordis yordis force-pushed the refactor-zitadel-server branch 5 times, most recently from e7881f8 to ba081ed Compare April 15, 2024 20:15
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

None yet

4 participants