-
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/auth architecture pt6 #282
base: v0.2.0
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@timothyrusso Please review this by Monday :) |
Except for the comment above, the other things are ok for me |
const currentVoyageTeam = voyageTeamMembers.find( | ||
(voyage) => voyage.voyageTeam.voyage.status.name === "Active", | ||
const user = useUser(); | ||
const voyageTeamAdapter = resolve<VoyageTeamClientAdapter>( |
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.
Can we define these adapters in separate files? This approach would make the codebase more scalable.
For example:
export const voyageAdapters = {
voyageTeamAdapter: resolve<VoyageTeamClientAdapter>(TYPES.VoyageTeamClientAdapter),
...
} as const;
We could organize the files like this:
-resolve
--voyage.ts
--another_adapter.ts
...
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.
Definitely. I did have something like this in mind but just implemented like this because I'm assuming the way these classes get instantiated will be changed later anyways. Another reason I didn't do it now is because I wasn't entirely sure what would be a good way to do it until I see how the final structure will be with the entire modules folder that will be moved to the separate repo
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.
Definitely. I did have something like this in mind but just implemented like this because I'm assuming the way these classes get instantiated will be changed later anyways. Another reason I didn't do it now is because I wasn't entirely sure what would be a good way to do it until I see how the final structure will be with the entire modules folder that will be moved to the separate repo
@Dan-Y-Ko In my company's app we have dozens of UseCases (adapters in chingu's case), and the logic that you see in my previous comment is what we actually do
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.
Definitely. I did have something like this in mind but just implemented like this because I'm assuming the way these classes get instantiated will be changed later anyways. Another reason I didn't do it now is because I wasn't entirely sure what would be a good way to do it until I see how the final structure will be with the entire modules folder that will be moved to the separate repo
@Dan-Y-Ko In my company's app we have dozens of UseCases (adapters in chingu's case), and the logic that you see in my previous comment is what we actually do
I'm pretty sure this would be part of the refactors you wanted to do with how the adapters are called. I'm doing it this same way in all the other files
Description
This task did some refactors to the logic for handling voyage access in the sidebar. You shouldn't see anything differently visually, this was just code refactors. Note that none of the pages are actually functional currently (they will work when those get finished refactoring)
Issue link
Fixes # (issue)
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: