-
Notifications
You must be signed in to change notification settings - Fork 2
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/hexagonal pattern poc v2 #235
Conversation
…ingu-dashboard into feature/exagonal-pattern-poc
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
so one issue I saw. The cache and token isn't being enforced here. I am pushing a "solution" but it's not great because it's fragile. However, I'm not sure what else could be done....would be possible if classes were useable. But that also comes with an issue of adding more code |
} from "@/modules/restApi/application/entities/restApiParams"; | ||
import { type RequestOptions } from "@/modules/restApi/application/entities/requestOptions"; | ||
|
||
type NextJsAuthRequestOptions = Required< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we handle all these types and interfaces in a separate file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My general rule of thumb is to keep types that are local in the relevant file, and global types in its own global file. The types here are only gunna be used by this adapter so I think it's fine to just keep it in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this approach; I prefer keeping types or interfaces in separate files to make management easier, but it's up to you
@@ -53,23 +59,43 @@ export interface IdeationVoteResponse extends IdeationResponse { | |||
projectIdeaId: number; | |||
} | |||
|
|||
const nextJsRestApiAdapter = new NextJsRestApiAdapter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, do we prefer to keep all the types/interfaces in the same file with the implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure what you mean by this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I mentioned before, I prefer keeping types/interfaces separate from the actual implementation/logic
const addIdeationAsync = () => | ||
POST<AddIdeationBody, AddIdeationResponse>( | ||
`api/v1/voyages/teams/${teamId}/ideations`, | ||
const addIdeationUseCase: NextJsAddIdeationAdapter = new AddIdeationUseCase( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the "const { Class } = resolveInjection()" (server side) method to import the class from the tsyringe container and use it here or in a component.
Or we can use "const { walletUseCases } = useInjection()" (client side).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a stupid example from another project:
// CLIENT SIDE
const { classUseCase } = useInjection();
useEffect(() => {
getData();
}, []);
const getData = async () => {
const response = await classUseCase().getUserData();
if (response.success) {
// do something
} else {
// do something
}
};
// SERVER SIDE
const { classUseCase } = resolveInjection();
const getData = classUseCase.getUserData();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we resolve the dependency of that usecase? For example in the add ideation usecase, it needs to inject the ideationApi (type ideationApiPort)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as you can see I instantiate it like const addIdeationUseCase = new AddIdeationUseCase(ideationApiPort);
Here's the flow: component -> primary adapter (implements the port) -> usecase -> secondary adapter (implements the port) See the note I wrote up above about issues with implementing the primary port The add ideation function in the service file is acting as the primary adapter here, the file naming could be changed if necessary |
I did some refactoring. I just split up the service file into a "server action" and an adapter. The adapter is written as a class and properly implements the port. I also managed to get the token and cache enforced in the server action. We do need to enforce that ourselves in the adapter, by making the token and cache required in the parameter) but that's a given, I think. So with this refactor, the server action calls the method in the adapter and handles other nextjs specific stuff. The adapter calls the usecase, and it goes from there. Here's the flow user performs some kind of action -> server actions is called -> client adapter (primary) is called -> appropriate usecase is called -> server adapter (secondary) is called Honestly, this has so many abstractions but I guess it is what it is |
…tor/hexagonal-pattern-poc-v2
b2c3404
to
2f622a6
Compare
I only did the implementation for the add ideation because I didn't want to go too far before deciding this is a good implementation. Would it be easier to understand if I did the entire ideation stuff? |
I think this could be confusing for anyone looking at the project in the future. I felt my first implementation was a bit simpler to follow with just three layers (implementation, interfaces, and use cases). I get the logic now, but this is just something to keep in mind for those working on the project later. We’ll need a brief documentation to explain this |
It's fine for now. I'm working on fixing the issue with class instantiation, and then we can migrate the other methods |
hexagonal architecture updated
some notes:
I first changed it to follow this folder structure
couple reasons why I didn't like this
so the change is what I like better
One other issue I came into was properly implementing the primary port (the primary adapter). Ideally it'd be the service acting as the adapter but first problem is we can't use classes in next with server component / actions. The second issue was that I couldn't really get the transformation to work while also keeping the interface in the port intact. The only solution I could think of is to return the normal response (instead of async response) from the adapter / service and add another layer to do the response transformation but Idk if there's much benefit to this. Instead I just decided to have the usecase implement the port, which I don't think 100% aligns with the architecture but I think it's fine, given the restraints with the tech stack.
I couldn't get the tsyringe resolve to work in the component so I instantiated the classes manually.
btw, I didn't go over the changes in depth because I figured you'd know this already but if you need to me to go over it, I can