-
-
Notifications
You must be signed in to change notification settings - Fork 18
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: add easly accessable handler in order to make unit tests easier #73
feat: add easly accessable handler in order to make unit tests easier #73
Conversation
|
Thanks! Unfortunately, I don't think that this is a pattern that I think this library should encourage, as it encourages to write handlers with complex obscure parameter and return types, instead of having clean and easy interfaces that can be defined explicitly next to a handler function. |
Ok, here is my use case. export const handle = composeHandler(
middlewareErrorHandler(),
middlewareLogger(),
middlewareJsonParser(),
middlewareValidator(lambdaSchemaPackageAdd),
middlewareLoadDependencies(["repositoryPackage", "integrationNpmJs"]),
async (event) => {
const existingPackage = await event.dependencies.repositoryPackage.findPackage({
packageName: event.jsonBody.packageName,
});
if (existingPackage !== null) {
throw new ConflictError("package must not already exists in database");
}
const packageInfo = await event.dependencies.integrationNpmJs.findPackageInfo({
packageName: event.jsonBody.packageName,
});
if (packageInfo === null) {
throw new ConflictError("package.packageName must exists in npm registry");
}
await event.dependencies.repositoryPackage.createPackage({
packageName: event.jsonBody.packageName,
packageStatus: event.jsonBody.packageStatus,
});
return awsLambdaResponse(StatusCodes.OK, {}, {});
},
); As you can see I'm doing DI through middleware and dependencies are available in event variable. In order to unit test my handler I need to access it directly without middlewares. export const handler = async (event) => { // types are gone here :(
const existingPackage = await event.dependencies.repositoryPackage.findPackage({
packageName: event.jsonBody.packageName,
});
if (existingPackage !== null) {
throw new ConflictError("package must not already exists in database");
}
const packageInfo = await event.dependencies.integrationNpmJs.findPackageInfo({
packageName: event.jsonBody.packageName,
});
if (packageInfo === null) {
throw new ConflictError("package.packageName must exists in npm registry");
}
await event.dependencies.repositoryPackage.createPackage({
packageName: event.jsonBody.packageName,
packageStatus: event.jsonBody.packageStatus,
});
return awsLambdaResponse(StatusCodes.OK, {}, {});
};
export const handle = composeHandler(
middlewareErrorHandler(),
middlewareLogger(),
middlewareJsonParser(),
middlewareValidator(lambdaSchemaPackageAdd),
middlewareLoadDependencies(["repositoryPackage", "integrationNpmJs"]),
handler,
); In my unit test I mock all dependencies and incoming data. I would love to see handler to be inside If you would like to reject this PR it's fine, I will create fork just for myself. |
Thanks! I've thought a bit about this, and would recommend to just add explicit types. The |
To be honest I don't really understand your point of view. |
If someone would end up in this thread here is a package I prepared for with this change and also using |
Thanks! I could add a link in a "related project" section of the README if you want :) |
Closes #
Checklist:
Previously in order to unit test handler I had to put it outside of
composeHandler
and types were gone.In order to make handler unit testable with fully working type system I assigned a variable to the function itself (not a common scenario) at the end of composeHandler.