Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

⚡ Improve find query performance and add adapterFactory with options #12

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dawidreedsy
Copy link

This code is taken from admin repo and simplified

@dawidreedsy dawidreedsy mentioned this pull request Nov 8, 2023
@dawidreedsy dawidreedsy force-pushed the improve-find-performance-find branch from 522309b to b37631f Compare November 8, 2023 09:56
@dawidreedsy dawidreedsy marked this pull request as ready for review November 8, 2023 10:15
@dawidreedsy dawidreedsy force-pushed the improve-find-performance-find branch from b37631f to 9e80449 Compare November 8, 2023 10:16
Copy link

@alecgibson alecgibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a test for this please

@@ -1,6 +1,11 @@
import escape from 'escape-regexp'
import mongoose from 'mongoose'

const FIND_ONE_FIELDS = [
'_id',
'uuid',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dawidreedsy
Copy link
Author

Test already exists

it('searches by id', async () => {
const users = await factory.createMany('user', 10)
const resource = new Resource(User)
const limit = 5
const offset = 0
const idFilters = new Filter({ _id: users[0]._id.toHexString() }, resource)
const returnedItems = await resource.find(idFilters, { limit, offset })
expect(returnedItems.length).toEqual(1)
expect(returnedItems[0]).toBeInstanceOf(BaseRecord)
})

@dawidreedsy dawidreedsy force-pushed the improve-find-performance-find branch from 48f511a to 0e72eb2 Compare November 8, 2023 12:09
src/resource.ts Outdated Show resolved Hide resolved
src/database.ts Outdated Show resolved Hide resolved
@dawidreedsy dawidreedsy force-pushed the improve-find-performance-find branch from 0e72eb2 to 072c9ca Compare November 8, 2023 12:29
@dawidreedsy dawidreedsy removed the request for review from alecgibson November 8, 2023 12:30
@dawidreedsy dawidreedsy force-pushed the improve-find-performance-find branch from 072c9ca to 3b3b93d Compare November 8, 2023 12:31
@dawidreedsy dawidreedsy changed the title ⚡ Improve find query performance ⚡ Improve find query performance and add adapterFactory with options Nov 8, 2023
@dawidreedsy dawidreedsy force-pushed the improve-find-performance-find branch from 3b3b93d to abb3f6c Compare November 8, 2023 12:34
@dawidreedsy dawidreedsy force-pushed the improve-find-performance-find branch from abb3f6c to 0c85377 Compare November 8, 2023 12:35

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this will do even bigger change especially in tests

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@alecgibson alecgibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've moved to ESM, so can you please rebase/rework on top of upstream?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants