-
Notifications
You must be signed in to change notification settings - Fork 94
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
Refactoring to achieve Vercel Edge (and other runtimes) #11
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 195 187 -8
Branches 20 20
=========================================
- Hits 195 187 -8
|
.eslintrc.json
Outdated
}, | ||
{ | ||
"from": "infrastructure", | ||
"allow": ["service-interfaces", "repository-interfaces", "entities"] | ||
}, | ||
{ | ||
"from": "use-cases", | ||
"allow": ["entities"] | ||
"allow": ["entities", "ioc"] |
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.
You should not allow to use the service locator / resolver from everywhere as it will be hard to keep the constraints on the dependency direction rules + it is considered as an anti-pattern normally to use it everywhere. Consider using it at the top level would be better and keep DI by constructore elsewhere. So you don't hide the dependencies and avoid mistakes. Wdyt?
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.
And pass the services/repos through args? I need them in the use cases because use cases interact with repos & services.
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.
You can use the service locator to get your controller (it needs your use case, so the use case will already be build with its own dependencies). Then you can use DI using constructor if it is a class or DI using higher order functions if it is a function. Something like this:
Controller:
interface Dependencies {
authenticationService: IAuthenticationService;
createTodoUseCase: ICreateTodoUseCase;
}
export const CreateTodoController = ({authenticationService, createTodoUseCase}: Dependencies) => ({
async createTodo(input: any, sessionId: string | undefined): Promise<ReturnType<typeof presenter>> {
return await startSpan({name: "createTodo Controller"},
async () => {
if (!sessionId) {
throw new UnauthenticatedError("Must be logged in to create a todo");
}
const {user} = await authenticationService.validateSession(sessionId);
const {data, error: inputParseError} = inputSchema.safeParse(input);
if (inputParseError) {
throw new InputParseError("Invalid data", {cause: inputParseError});
}
const todo = await createTodoUseCase.execute(data, user.id);
return presenter(todo);
},
);
}
});
and Use case:
interface Dependencies {
todosRepository: ITodosRepository;
}
export const CreateTodoUseCase = ({todosRepository}: Dependencies) => ({
async execute(input: { todo: string }, userId: string): Promise<Todo> {
return startSpan(
{name: "createTodo Use Case", op: "function"},
async () => {
// HINT: this is where you'd do authorization checks - is this user authorized to create a todo
// for example: free users are allowed only 5 todos, throw an UnauthorizedError if more than 5
return await todosRepository.createTodo({
todo: input.todo,
userId,
completed: false,
});
},
);
}
});
Here I return an object with the named functions like create createTodo and execute but you can also return directly the async functions if you want (just a convention we have in my team).
This way:
- your dependencies are clearly identified
- you know that you point to the ports and not directly to the adaptors
- you can use eslint boundaries because it will check the imports of your dependencies which are the interfaces
- you can add the constraint to use the service locator only to inject your controllers in your app and nowhere else which will be safer on big projects with lots of devs not familiair with CA deps rules.
Note: I think that you can be able to move the startSpans when resolving as it is not a use case concern imo
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.
That's actually pretty smart! Put use cases under IoC and instantiate them with the dependencies included. I'll def look into this. Thanks!
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.
Done! Really like how this looks like with ioctopus
.
@@ -1,27 +1,17 @@ | |||
import "reflect-metadata"; | |||
import { afterEach, beforeEach, expect, it } from "vitest"; | |||
import { expect, it } from 'vitest'; |
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.
If you want to remove the import of vitest everytime, you can use the globals:
import { defineConfig } from 'vitest/config'
export default defineConfig({
test: {
globals: true,
},
})
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.
oh didn't know that. thanks!
inversify
and implementing basic IoCinversify
and implementing evyweb/ioctopus
inversify
and implementing evyweb/ioctopus
inversify
and implementing @evyweb/ioctopus
P.S. I had to delete all sessions, users, and todos from the Vercel deployment because we're changing the hashing library so it'll always produce different hashes. |
inversify
and implementing @evyweb/ioctopus
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
At this time, we have two things preventing us from running on Verce's Edge runtime and having middleware:
inversify
@node-rs/argon2
After putting some thought into
inversify
and taking into account the issue of not being able to use this config in edge / middleware, I decided to completely ditchinversify
and implement evyweb/ioctopus.I did try
typed-inject
, but personally I didn't like the API. I had to chain every "register" method (can't break the registrations and pass the "injector"). Because I usedinversify
until this point,ioctopus
felt a lot more similar thantyped-inject
so I simply went withioctopus
.I've also registered the controllers and use cases in the container. Why? Because it's just safer, and we let the dependency resolving to the library itself. We only manually resolve the controllers at the page/action/API handler level. Controllers, use cases, services, and repositories get their dependencies automatically resolved by the library and passed through either constructor or arguments.
In regards to
argon2
, I refactored it to usebcrypt-ts
instead.bcrypt-ts
works on all runtimes.I'll leave this PR open for discussions, so please check out the changes and let me know what you think and if I'm missing something.
Thanks!
Closes #7