[#IP-84] Change strict configuration to true #20
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20 +/- ##
==========================================
+ Coverage 87.70% 87.97% +0.26%
==========================================
Files 22 22
Lines 431 424 -7
Branches 43 42 -1
==========================================
- Hits 378 373 -5
+ Misses 52 50 -2
Partials 1 1
Continue to review full report at Codecov.
|
| throw new Error(`Unexpected object: ${toString(x)}`); | ||
| }; | ||
| // tslint:disable:no-any | ||
| const createUnexpecterError = (x: any): Error => |
There was a problem hiding this comment.
The whole point of assertNever is to have an assignment to a variable of type never; that means no other type can be passed to such function except never. This is an important mechanism to mark death branches in code (that is: conditions we assert are impossible to happen), and it is at foundation of every exhaustive check.
That said, the return type is not important because the functions either is never called or TS fails with wrong arguments error.
What about runtime? If this functions is executed, it means the variable contains a value which shape differs from the declared type. As this is a problem that can make the whole execution inconsistent, it's safer to throw an exception and let the error to bubble up.
| @@ -87,7 +88,7 @@ export const getCallNHServiceActivityHandler = ( | |||
| return failure(e.message); | |||
| }); | |||
| default: | |||
| assertNever(message); | |||
| throw createUnexpecterError(message); | |||
|
We needed to undergo a huge refactor in 0d28679 in order to make the newly-introduced
This wasn't intended but needed in order for input and output types to be fully instantiated. We're loosing the hard coupling between input and output of an activity - other said: an orchestrator can create an arbitrary pair input/output for an activity, and the type system is not covering us from such errors. These would be gross errors and easy to spot at code review, however I'm not happy with the result and I commit to improve such type definitions. Luckily, we didn't have to touch the implementation. |
| notifyMessageActivity: CallableActivity<NotifyMessageActivityBodyImpl>; | ||
| notifyMessageActivity: CallableActivity< | ||
| NotifyMessageActivityInput, | ||
| NotifyMessageActivityResultSuccess |
There was a problem hiding this comment.
Why the need of specifying also the success type, since it's a simple ActivityResultSuccess?
There was a problem hiding this comment.
Just for more consistency. As we lost the ActivityBody here, there's no other mechanism that is ensuring us to use the very same output type of the activity.
Thinking about making S non-optional for callableActivity....
There was a problem hiding this comment.
We could think about exporting the callableActivity directly from the activity itself, what do you think?
So it's up to the activity expose it with right types
There was a problem hiding this comment.
Or more: we could just scaffold all types so that we don't have to use so much generics
Co-authored-by: gquadrati <75862507+gquadrati@users.noreply.github.com>
List of Changes
Set the stirct configuration to true.
Fixed the compilation errors caused by the new settings.
Motivation and Context
The configuration strict set to true will help to reduce errors enforcing a less forgiving TS compiler.
How Has This Been Tested?
The changes have been tested running again the provided unit tests.
Screenshots (if appropriate):
Types of changes
Checklist: