feat: Add support for custom failure converters #887
Conversation
| @@ -1,3 +1,4 @@ | |||
| import { DefaultFailureConverter, FailureConverter } from '../failure'; | |||
There was a problem hiding this comment.
I have a bit of a circular import problem if I do this in Python and it can be a bit confusing even here in TS. To have converter/data-converter.ts depend on failure.ts which depends on converter/payload-converters.ts. This is fine in TS because you are breaking this stuff out into so many files (which I don't necessarily agree with), but it's still confusing to have thing in subdir depend on thing in general dir that depends on thing back in subdir.
I think it might be clearer to not have failure.ts depend on anything from converter and move anything related to error conversion (i.e. needing error and converter stuff) into things in this directory. But if you don't want to create a converter/failure-converter.ts, no worries.
I think in Python at least, I'll have temporalio.converter.FailureConverter right next to temporalio.converter.PayloadConverter and temporalio.converter.DataConverter, and remove all temporalio.converter imports from temporalio.exceptions. Today temporalio.exceptions depends on temporalio.converter but I think I'm gonna flip that now that DataConverter will accept a FailureConverter (class). I haven't written it yet, but it makes the most sense. Thoughts welcome.
There was a problem hiding this comment.
But if you don't want to create a converter/failure-converter.ts, no worries.
This is what I did initially and ended up moving the code into failure.ts, I can definitely move the converter code to its own file.
There was a problem hiding this comment.
What you have here is fine by me if you don't mind the dependency graph it creates.
In Python I can't have DataConverter depending on a separate-package FailureConverter that depends back on the data-converter package for PayloadConverter. So I have to have the FailureConverter with the other converters.
packages/common/src/failure.ts
Outdated
| * It is recommended to use the {@link DefaultFailureConverter} and not attempt to customize the default implementation | ||
| * in order to maintain cross language failure serialization compatibility. |
There was a problem hiding this comment.
This comment should explain why this is exposed at all then, and when you might need to customize it
There was a problem hiding this comment.
There shouldn't ever be a need for that
There was a problem hiding this comment.
Then why is this exposed at all? If that's the case I find this comment even more confusing
There was a problem hiding this comment.
To give the full flexibility, we could have just exposed a boolean to decide whether or not to encode failure attributes but that would limit power users that might want to implement their own failure serialization logic
| /** | ||
| * Converts an error to a Failure proto message if defined or returns undefined | ||
| */ | ||
| export function optionalErrorToOptionalFailure( |
There was a problem hiding this comment.
These are technically backwards-incompatible alterations right? I support removing them of course and I don't expect users ever used them.
packages/common/src/failure.ts
Outdated
| ); | ||
| // Don't apply encodedAttributes unless they conform to an expected schema | ||
| if (typeof attrs === 'object' && attrs !== null) { | ||
| const { message, stack_trace } = attrs; |
There was a problem hiding this comment.
Does this fail if our attributes are not present? I don't think it should.
There was a problem hiding this comment.
It does not fail, it will fail if the payload converter cannot convert from payload though which I think is reasonable.
There was a problem hiding this comment.
👍 My mistake, I assumed destructuring failed on missing key
Can't test with temporalite yet, tested locally against server 1.18 in the meantime. Leaving the test as is (skipped) for now since we should be getting a new temporalite release soon.