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/hexagonal pattern poc v2 #235

Draft
wants to merge 34 commits into
base: dev
Choose a base branch
from

Conversation

Dan-Y-Ko
Copy link
Collaborator

@Dan-Y-Ko Dan-Y-Ko commented Sep 4, 2024

hexagonal architecture updated

some notes:

I first changed it to follow this folder structure

application
domain
infrastructure

couple reasons why I didn't like this

  • It felt too much like layers to me. Hexagonal architecture in implementation is actually quite similar (if not the same) as n-tier architecture. After a lot of reading, it seems the main difference is conceptually. Instead of data flowing down in layers, the ports and adapters is emphasized. I won't talk more about this as I'm sure you already know.
  • With the concept of primary and secondary ports/adapters I found so many conflicting info about what goes where so that's why I decided to refactor again
  • It became very confusing to me and hard to keep track of the different ports/adapters when it was separated by "layers"

so the change is what I like better

application
ports
adapters

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

Copy link

vercel bot commented Sep 4, 2024

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

Name Status Preview Comments Updated (UTC)
chingu-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 1, 2024 6:09am

@Dan-Y-Ko Dan-Y-Ko mentioned this pull request Sep 4, 2024
12 tasks
@Dan-Y-Ko
Copy link
Collaborator Author

Dan-Y-Ko commented Sep 4, 2024

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<
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator

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(
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator

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(
Copy link
Collaborator

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

Copy link
Collaborator

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();

Copy link
Collaborator Author

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)

Copy link
Collaborator Author

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);

@Dan-Y-Ko
Copy link
Collaborator Author

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

@Dan-Y-Ko
Copy link
Collaborator Author

Dan-Y-Ko commented Oct 1, 2024

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

@Dan-Y-Ko Dan-Y-Ko force-pushed the refactor/hexagonal-pattern-poc-v2 branch from b2c3404 to 2f622a6 Compare October 1, 2024 06:07
@Dan-Y-Ko
Copy link
Collaborator Author

Dan-Y-Ko commented Oct 1, 2024

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?

@timothyrusso
Copy link
Collaborator

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

@timothyrusso
Copy link
Collaborator

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?

It's fine for now. I'm working on fixing the issue with class instantiation, and then we can migrate the other methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants