channels-1094: Refactoring diffs

[pmunin]

A summary of your pull request, including the what change you're making and why.

Testing

Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.

• Added unit tests for new functionality
• Tested end-to-end using the local server
• [Segmenters] Tested in the staging environment

[cogwizzle]

After working with this abstraction I like that it works in a similar way to how Rust Results works, but I fear that it does deviate from the normal JavaScript standard way of operating quite significantly. I'm not sure how transparent this abstraction is in comparison to the traditional try catch statements.

I'm not saying we can't use this, but what I do think is this pattern needs to be more carefully considered before we introduce it. I think if we do decide to go this route we should likely include this proposed pattern to a larger audience as well.

[cogwizzle]

I had to add this in order to prevent errors as result can be undefined according to the type. We could probably fix this type issue, but I think it is due to the ValueOrError type.

[cogwizzle]

I don't hate the reusable nature of this function. It feels a bit out of place in a few ways.

1. I think the function is doing to much. If we want to think about this in an abstract way we should have a function that handles the retrieval of the cache and then have a strategy function to route to either returning the cached value or another function which executes the call and then adds it to a cache. Writing test for this function would be very difficult.

2. This feels like a very OOP approach to this problem where we've written a generic class in one function. Should we consider building that class?

[iamsuneeth]

As a first time reviewer ( and hence inexperienced) of this repo:

I second @cogwizzle 's opinion here. While the function is keeping itself generic, I feel it is doing too much and the multi level getOrCatch conditions seems to be affecting the readability of the code. The benefit of having a getOrCatch over multiple try catch statements is not clear enough to me.

We could go with a generic class as @cogwizzle suggested.
I was also thinking whether we could inject a cache to this function and then this function can be used to handle any caching scenario in future and is not tied to engageDestinationCache and may help in doing away with definite assignment assertion. Probably an overkill if we don't anticipate a reuse of this function in future though

[pmunin]

feel free to suggest your implementation that will keep the same clarity of where the error came from

[cogwizzle]

Fixed the last batch of TypeScript issues. I think that if we're going to use patterns like this one we should be very strict with types and ensure that we're writing completely type complete code even though it increases the verbosity of our code. The language patterns you've introduced mirror very strongly typed languages and the more TS hacks we use the more likely it is to break.

[pmunin]

@cogwizzle plz see comments. I didn't find bugs you were talking about, let's zoom after meetings

[pmunin]

too much code to make typescript happy for questionable value coming out of it

Suggested change

	const engageDestinationCache = this.engageDestinationCache
	if (!engageDestinationCache) return createValue()
	const cacheRead = await getOrCatch(() => engageDestinationCache.getByKey(key))
	if (!this.engageDestinationCache) return createValue()
	const cacheRead = await getOrCatch(() => this.engageDestinationCache!.getByKey(key))

[pmunin]

same here - too much code and complexity for questionable value. increases cognitive load is the price we pay for perfectionism :)

Suggested change

	const cacheReadValue = 'value' in cacheRead ? cacheRead.value : undefined
	const cacheReadValue = cacheRead.value

[pmunin]

same here. this one line
const { value: parsedCache, error: parsingError } = getOrCatch(() => serializer.parse(cacheRead.value!)) became this large paragraph with bunch of explicit typings just to make typescript happy

[pmunin]

same here. Also cache_hit metric tag does not make sense in this implementation. I think cache_hit should include erroneous value which we could not parse

[pmunin]

bug in the code and notably happy typescript didn't help :-)

Suggested change

	const result = getOrCatch(() => this.engageDestinationCache?.setByKey(key, stringified.value))
	if ('error' in result && result.error) {
	this.logError('cache_saving_error', { key, error: result.error })
	this.statsIncr('cache_saving_error')
	}
	const result = await getOrCatch(() => this.engageDestinationCache?.setByKey(key, stringified.value))
	if ('error' in result && result.error) {
	...
	}

also var name result is confusable with line 193. And again, this is less readable code, comparing to what it was:

      const { error: cacheSavingError } = await getOrCatch(() => this.engageDestinationCache!.setByKey(key, stringified.value!)
      if (cacheSavingError) {
          ...
      }
      

[pmunin]

looks unnecessary to add so much types, i'd just keep it as it was

Suggested change

	type Ok<T> = { value: T }
	type Err<E> = { error: E }
	type ValueOrError<T> = Ok<T> | Err<T>
	type ValueOrError<T> = { value?: T; error?: any }

[codecov]

Codecov Report

Attention: Patch coverage is 96.82540% with 2 lines in your changes missing coverage. Please review.

Project coverage is 77.89%. Comparing base (d682103) to head (84f4fe8).
Report is 73 commits behind head on channels-1094.

Files	Patch %	Lines
...ssaging-twilio/__tests__/__helpers__/test-utils.ts	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                @@
##           channels-1094    #2324      +/-   ##
=================================================
+ Coverage          77.47%   77.89%   +0.42%     
=================================================
  Files                905      944      +39     
  Lines              15584    17384    +1800     
  Branches            2853     3372     +519     
=================================================
+ Hits               12074    13542    +1468     
- Misses              2574     2904     +330     
- Partials             936      938       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pmunin]

Closing towards #2325