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

feat: added strong typed orama schema #470

Merged
merged 5 commits into from
Sep 19, 2023

Conversation

H4ad
Copy link
Contributor

@H4ad H4ad commented Aug 27, 2023

Closes #456
/claim #456

This PR turns the Orama completely type-safe, below some examples of how to use:

Schema

Now, if you want to have the type from the result of your schema, you can do the following:

import { Schema } from '@orama/orama'

const mySchema = {
  title: 'string'
} as const;
type MySchemaDocument = Schema<typeof mySchema>;

Be careful with as const, it is required to be able to correctly infer your schema, if you didn't make it const, your document will be inferred to never.

Create

By default, create will create the correct typings from schema automatically:

const db = await create({
  schema: {
    quote: 'string',
    author: 'string',
  },
})

The return type will be:

image

It also works for more complex schemas:

image

If you didn't customize the index, sorter, or documentStore, it will infer to the default implementations.

Insert & Update

During the insert, all the properties are optional and you can also specific custom properties, like:

const oldDocId1 = await insert(db, {
  quote: "Life is what happens when you're busy making other plans",
  author: 'John Lennon',
  customProp: 'Olá'
})

But, if you specify a wrong value for some value in the schema, for example passing a number to quote, you will receive an error:

image

Search

With search, you will have two behaviors, by default the result type will be the typed-schema:

image

But, in cases where the result will be too much different from the typed-schema, you can do the following:

image

Also, now we automatically suggest options for groupBy.properties, sortBy.property and where, like this:

image

image

Currently, nested properties such as doc.title are not suggested even if you have schema support, but you can specify them without problems.

Sharing Typed Orama Instance

In cases where you need the type for Orama, you can do the following:

import { Schema, TypedDocument } from '@orama/orama'

const mySchema = {
  title: 'string'
} as const;

type MyOrama = Orama<typeof mySchema>;

// the following will be the same as Schema<typeof mySchema>;
type MySchemaDocument = TypedDocument<MyOrama>

Hooks

The hooks from components also are correctly typed based on the schema:

image

But is not possible to infer extra properties, like when your schema has just title but in your document you also insert description.

In those cases, you should do the following:

const db = await create({  
  schema: {
    strings: 'string[]',
    num: 'number[]',
    b: 'boolean[]',
  },
  components: {
    beforeInsert: (orama, key, doc) => {
      type MyDoc = { strings: string[], num: number[], b: boolean[], extraProperty: string };
      const typedDoc = doc as unknown as MyDoc
    }
  }
})

But nothing stops you from just grab the property you want, like:

console.log(doc!.extraProperty);

It just will just be typed as any.

Breaking Changes

I needed to upgrade the typescript to 5.0.0 because of the new modifier const, this makes it easier for the users to specify the schema without the need of doing { schema: { title: 'string' } as const }.

@vercel
Copy link

vercel bot commented Aug 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
orama-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 19, 2023 0:51am

@micheleriva
Copy link
Member

Hi @H4ad , thanks! Would you be able to fix conflicts?

Copy link
Member

@micheleriva micheleriva left a comment

Choose a reason for hiding this comment

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

I think this mostly looks good to me. I think we will need to update the docs as well if we want to use as const when defining the schema. But I really with we could get rid of it actually... so we can avoid such a drastic change.

@H4ad will this work on TypeScript <5?

@allevo could you also take a look at this?

@H4ad H4ad marked this pull request as draft September 5, 2023 23:47
@H4ad
Copy link
Contributor Author

H4ad commented Sep 5, 2023

@micheleriva I'm still working on types, and cleaning the tests (some of them passes but the types is actually broken).

It could work on <5 but I don't think we will get the same results and API that we have now since create will not be able to infer correctly the types of the schema.

@H4ad H4ad marked this pull request as ready for review September 6, 2023 00:11
@H4ad
Copy link
Contributor Author

H4ad commented Sep 6, 2023

Now the code is stable enough, I also added better types for search (I missed some opportunities to correctly type), so now I think the types are good enough.

@allevo
Copy link
Collaborator

allevo commented Sep 6, 2023

If I understood correctly, this PR propagates the schema definition to the insert/insertMultiple/search methods: amazing!
We should mention something in the documentation.

Copy link
Collaborator

@allevo allevo left a comment

Choose a reason for hiding this comment

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

We should update the documentation as well
Something like "orama is fully typed so the document type is inferred from the schema" in create, insert, and search pages.

WDYT?

@H4ad
Copy link
Contributor Author

H4ad commented Sep 14, 2023

@allevo I have no skills to write good documentation about this, I think the documentation should be in another PR as we did for internal IDs, since this is a breaking change and also very important, this probably will deserve a blog post too.

@allevo allevo self-requested a review September 14, 2023 07:11
Copy link
Collaborator

@allevo allevo left a comment

Choose a reason for hiding this comment

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

ok I'll care about docs! No problem.

LGTM

Comment on lines 47 to 48
const s = await search(db, { term: 'fish' })
const year: any = s.hits[0].document.year // not inferred, so it's any
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not inferred but you can make it type-safe by passing the interface with that field, like: search<WithYearField>

Copy link
Collaborator

Choose a reason for hiding this comment

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

good point. I'm changing it!

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems a test is missing for this case. Could you add it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests that you have on Orama don't test for types, you can literally break the types and the tests will not break, they only break when the assert is false (at least that was the behavior I had when I was changing the Orama tests).

But I can add it by the end of the day.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, till now, we don't have that kind of test. But with this PR, type types are guaranteed. So, a little test on that would be amazing!
At least, we document this capability in the test suite also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But probably this will require a new project and command to test only the types, separated from the default tap tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

tsd seems to do the job greatly.
I'll go for just a new command run after the test suite. it should work.

@@ -19,40 +19,6 @@ Orama is a fast, batteries-included, full-text and vector search engine entirely

Get started with just a few lines of code:
Copy link
Member

Choose a reason for hiding this comment

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

@allevo please remove this line as well

Copy link
Member

@micheleriva micheleriva left a comment

Choose a reason for hiding this comment

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

LGTM. Just a little fix for @allevo

@micheleriva
Copy link
Member

Hi @H4ad, we only need a few conflict fixes to get this one merged. Thanks!

@H4ad
Copy link
Contributor Author

H4ad commented Sep 19, 2023

@micheleriva @allevo what should be the type for enum? string, number? both?

@H4ad
Copy link
Contributor Author

H4ad commented Sep 19, 2023

Based on the tests, I leave it as string | number.

And we can merge this PR before #482, I don't want to resolve conflicts for the third time 😓

@micheleriva
Copy link
Member

@H4ad you're right. Sorry if it took so long for us to merge, it's a pretty complex one. Thanks for your work!

@micheleriva micheleriva merged commit ba29008 into oramasearch:main Sep 19, 2023
2 checks passed
@allevo
Copy link
Collaborator

allevo commented Sep 19, 2023

Thanks!

@H4ad H4ad deleted the feat/typing branch September 19, 2023 10:16
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.

I am unable to specify a data type for my index, and that compromises the type safety
3 participants