fix: move runInTransaction to ViewerContext

[wschurman]

Why

This has been suggested by @jkhales and @ide a few times as a better pattern. At its core this does the same thing as the previous implementation, it just removes the unnecessary association with concrete entity types.

How

Move the methods to ViewerContext. Note that this will most likely be used in methods in ViewerContext subclasses in the following manner:

// in ExpoViewerContext.ts

async runInPostgresTransactionAsync<TResult>(
  transactionScope: (queryContext: EntityTransactionalQueryContext) => Promise<TResult>
): Promise<TResult> {
  return await this.runInTransactionForDatabaseAdaptorFlavorAsync(
    DatabaseAdapterFlavor.POSTGRES,
    transactionScope
  );
}

so that callsites don't need to specify the DatabaseAdapterFlavor.

Questions for Reviewers

Do you think it's worth added these DatabaseAdapterFlavor-hardcoded methods to ViewerContext? Since the DatabaseAdapterFlavor is hardcoded in this package, it can be done cleanly. The upside is that consumers don't need to specify them. The downside is that the number of methods would grow if we add more DatabaseAdapterFlavors.

Test Plan

Run all tests.

[codecov]

Codecov Report

Merging #108 (e62f2fd) into master (01b3bfe) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #108   +/-   ##
=======================================
  Coverage   94.95%   94.96%           
=======================================
  Files          66       66           
  Lines        1624     1627    +3     
  Branches      195      177   -18     
=======================================
+ Hits         1542     1545    +3     
  Misses         80       80           
  Partials        2        2           

Flag	Coverage Δ
integration	94.96% <100.00%> (+<0.01%)	⬆️
unittest	94.96% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/entity/src/ReadonlyEntity.ts	100.00% <ø> (ø)
packages/entity/src/EntityCompanionProvider.ts	100.00% <100.00%> (ø)
packages/entity/src/ViewerContext.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01b3bfe...e62f2fd. Read the comment docs.

[jkhales]

nice. ✨

I would be curious to know what other flavors will be added. For example if MySQL is added then things would be similar enough for transactions that a runInSQLTransactionAsync method seems like the cleaner abstraction.

[wschurman]

I would be curious to know what other flavors will be added. For example if MySQL is added then things would be similar enough for transactions that a runInSQLTransactionAsync method seems like the cleaner abstraction.

We'd need one method per flavor just in case an application uses multiple flavors. This flavor concept as a whole though probably needs to be removed and just replaced with strings so that client applications can have as many "flavors" as they want and configure them however they like.

[ide]

Since the number of database flavors will likely stay small, having hardcoded methods seems good so that we don't need to pass the same parameter in everywhere. It also might help with type hints a bit if the Postgres query context were to be different than a MySQL query context.

Longer-term, if we have multiple flavors, I think we will want to automatically detect what flavors are used and just have a generic "runInTransactionAsync" method that creates the correct query contexts on the fly as needed, or at least warn if you started a Postgres transaction but tried to access MySQL. There might be cases where we need to let developers opt out (e.g. they want a PG transaction but not a MySQL one) but those cases seem rare.

[ide]

Might want to make getTableDataCoordinatorForEntity call this so that the behavior between the two methods stays in sync.