⚡ Improve find query performance and add adapterFactory with options#12
⚡ Improve find query performance and add adapterFactory with options#12dawidreedsy wants to merge 2 commits intomainfrom
Conversation
522309b to
b37631f
Compare
This code is [taken from admin repo](https://github.com/reedsy/reedsy-editor-admin/blob/a3ebb5d13d0adf7441d324928ad84804edd3ac0e/src/services/adminjs/resources/utilities/patch-find-one.ts#L1-L48) and simplified
b37631f to
9e80449
Compare
alecgibson
left a comment
There was a problem hiding this comment.
Can we have a test for this please
src/utils/convert-filter.ts
Outdated
|
|
||
| const FIND_ONE_FIELDS = [ | ||
| '_id', | ||
| 'uuid', |
There was a problem hiding this comment.
While _id is pretty standard, uuid less so. I think we should make this configurable somehow, which also lets us change the Admin app without having to change this library every time we want to add a new field to this.
|
Test already exists adminjs-mongoose/test/resource/find.spec.ts Lines 22 to 33 in 9e80449 |
48f511a to
0e72eb2
Compare
0e72eb2 to
072c9ca
Compare
072c9ca to
3b3b93d
Compare
3b3b93d to
abb3f6c
Compare
abb3f6c to
0c85377
Compare
There was a problem hiding this comment.
This feels like a big departure from upstream. It's nice to keep as closely aligned as possible in case upstream changes.
I think I'd expect to configure this per-resource, probably by augmenting ResourceOptions (maybe set in properties?
There was a problem hiding this comment.
I feel like this will do even bigger change especially in tests
There was a problem hiding this comment.
We already diverged quite a bit from master as we use version 3.0.3 thay are already on version 4 which is ESM only
There was a problem hiding this comment.
I'm beginning to wonder if we should jump the Admin app to ESM. It's relatively small, so might hopefully be one of the more painless apps to move
There was a problem hiding this comment.
As discussed let's not move it to ESM as it not as simple as it looks.
We already diverged from admin js (version 4 actually do very similar to what i do), so let me know if you sitll insit on implementing it using this:
I think I'd expect to configure this per-resource, probably by augmenting ResourceOptions (maybe set in properties?
alecgibson
left a comment
There was a problem hiding this comment.
We've moved to ESM, so can you please rebase/rework on top of upstream?
This code is taken from admin repo and simplified