Skip to content

Conversation

@ericcrosson-bitgo
Copy link
Contributor

This refactor changes the response code path to try encoding the
service function's response value with the apiSpec's response codec,
and if this errors returns a 500.

Previously we used the codec's type-guard as a test if the codec
can encode the service function's response value, but this step will
take extra time and there's no hard contract that guarantees us the
type-guard returning true really means that the codec will be able
to encode a value. Instead, we just try the dangerous deed and do our
best to catch errors.

@ericcrosson-bitgo ericcrosson-bitgo requested a review from a team May 20, 2022 00:39
}

expressRes.status(status).json(responseCodec.encode(payload)).end();
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proving that responseCodec isn't undefined is pretty gross in this implementation 😓 I used a ! because undefined has no encode method will still jump to the catch block

This refactor changes the response code path to try encoding the
service function's response value with the apiSpec's response codec,
and if this errors returns a 500.

Previously we used the codec's type-guard as a test if the codec
can encode the service function's response value, but this step will
take extra time and there's no hard contract that guarantees us the
type-guard returning `true` really means that the codec will be able
to encode a value. Instead, we just try the dangerous deed and do our
best to catch errors.
Copy link
Contributor

@bitgopatmcl bitgopatmcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@bitgopatmcl bitgopatmcl merged commit 0aabe69 into BitGo:beta May 20, 2022
@ericcrosson-bitgo ericcrosson-bitgo deleted the use-throw branch May 20, 2022 22:08
@github-actions
Copy link

🎉 This PR is included in version 1.0.0-beta.10 🎉

The release is available on npm package (@beta dist-tag)

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 0.2.0-beta.6 🎉

The release is available on npm package (@beta dist-tag)

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Jun 6, 2022

🎉 This PR is included in version 0.2.0-beta.2 🎉

The release is available on npm package (@beta dist-tag)

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 0.2.0-beta.6 🎉

The release is available on npm package (@beta dist-tag)

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants