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: bring asset response type into shared package #1361

Merged
merged 9 commits into from
Jun 12, 2024

Conversation

rico191013
Copy link
Contributor

@@ -0,0 +1,19 @@
export type Asset = {
Copy link
Contributor

Choose a reason for hiding this comment

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

are these types used in backend as well?

Copy link
Member

Choose a reason for hiding this comment

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

#1351
The Types shared here should be imported in the backend to be used as the response type of controllers,
and also in the frontend in order to import and not duplicate response types.

So i believe these types should also be added to the controllers?

@beniaminmunteanu
Copy link
Member

The shared package between backend and frontend should export:

  • CustomResponse

  • Interfaces for each return type
    So that Both the frontend and the backend are able to import and use the following construct:
    CustomResponse

As it is now currently used in places such as.

Each call from our FE to our BE should type the response with the above construct, instead of writing the response type in the FE.

@github-actions github-actions bot added the package: wallet/backend Wallet backend implementations label Jun 11, 2024
@beniaminmunteanu
Copy link
Member

Keeping in mind the other places that need to have the types moved to shared: #1351

My suggestion would be to have the interfaces in shared have a conventional name, like including "Response" in it such as "NameResponse"

So that in the "account" implementation for example we would have the "Account" type being used for the bussiness logic inside the backend, and that is not shared, but for the controller response type we would have "AccountResponse"

Wdyt?

@github-actions github-actions bot added the type: ci Changes to CI workflows label Jun 12, 2024
dragosp1011
dragosp1011 previously approved these changes Jun 12, 2024
@github-actions github-actions bot removed the type: ci Changes to CI workflows label Jun 12, 2024
@github-actions github-actions bot added the type: ci Changes to CI workflows label Jun 12, 2024
@rico191013 rico191013 merged commit 93ffac2 into main Jun 12, 2024
15 checks passed
@rico191013 rico191013 deleted the shared-wallet-asset branch June 12, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: wallet/backend Wallet backend implementations package: wallet/frontend Wallet frontend implementations type: ci Changes to CI workflows type: source Source changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Subtask] - Share types for Asset
3 participants