Conversation
a8ac359 to
3d89f3d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #441 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 109 107 -2
Lines 16142 15713 -429
Branches 1419 1412 -7
==========================================
- Hits 16142 15713 -429
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3d89f3d to
bced9f8
Compare
|
The main tradeoff that comes to mind is (lack of) multiple inheritance. Maybe it doesn't matter because entities are backed by only one type of database. |
bced9f8 to
429ae05
Compare
0938147 to
2a319a4
Compare
|
It's a valid tradeoff and one that if it is important enough for an application they can opt to just use the free functions added here instead of the base classes (they're equivalent as shown by the tests in the PR that adds the subclasses). For Expo's server, we use subclasses already for all our entities. It'd be cool if mixins were officially supported in TS/JS. The other option is a higher-order component function (closest thing to mixins), something like: But the key is that these approaches all require the free functions added here, and for now the subclass approach matches most closely to what the Expo server will need. |
Merge activity
|
429ae05 to
8f2c103
Compare
…nexLoader methods (#442) # Why This is part 2 of the refactor described in #441. It adds subclasses of Entity and ReadonlyEntity that supply the static `knexLoader` methods to keep the ergonomics of the extension but without the monkey-patching. # How Add subclasses, add test. # Test Plan Run test.
# Why Part 4 of #441. This reverts the test changes to use the better pattern, though both work equivalently and are both tested. # How Update tests. # Test Plan Run tests

Why
Asked claude extended thinking to analyze the
installExtensionsmonkey-patching strategy we went with in #410 and compare it to other typescript ORM libraries.It said essentially that it was ok, but most modern libraries choose not to do it that way, and instead recommended "free functions" (TIL about that term) to instantiate the loaders. (
knexLoader(BlahEntity, vc))Working with it even more, I came up with an even better method on top of free functions to keep the
BlahEntity.knexLoader(vc)syntax. The strategy is to:PostgresEntity/ReadonlyPostgresEntitysubclasses that application entities can extend that implement the static extension methods.BlahEntity.knexLoader(vc)syntax since most applications should use the subclasses (though it's not strictly necessary if they prefer the free function at callsites).How
Claude did most of this, but broadly speaking the strategy is to have the free functions, and then instead of patching the prototypes/classes for singleton caching, use a WeakMap where the core package singleton is the key and the value is the new knex-specific object.
Test Plan
Run all tests.