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

Key datastore property bleeds into typescript Type #173

Open
jacobg opened this issue Jul 19, 2019 · 11 comments
Open

Key datastore property bleeds into typescript Type #173

jacobg opened this issue Jul 19, 2019 · 11 comments
Labels

Comments

@jacobg
Copy link

jacobg commented Jul 19, 2019

Following the documentation on using gstore-node with Typescript, I had to define a custom Type which has a key property in the following way:

    type ChildType = {
      // It's not great that the type needs to bleed datastore semantics.
      // The type is supposed to be datastore-agnostic.
      // Probably a string id value is more appropriate.
      parentField: entity.Key
    }
    const ChildSchema = new gstore.Schema<ChildType>({
      parentField: { type: gstore.Schema.Types.Key, ref: ParentModel.entityKind }
    })
    const ChildModel = gstore.model<ChildType>('ChildType', ChildSchema)

I had tried using a string or a long type on the parentField, but it causes a compile error when calling new ChildModel({ ... }).

It would seem that the ChildType's parentField should be database-agnostic, and maybe an int | string union type. Can't the kind and namespace be inferred from the child entity itself?

@sebelga
Copy link
Owner

sebelga commented Sep 10, 2019

Hello,

I am not sure I am following you. Being able to declare parentField: entity.Key, when you call new ChildModel({ ... }) will allow us to get Typescript error if we don't provide a Key type. Which is what we expect.

Could you elaborate with an example of the issue? gstore is meant to be used with the Datastore, why would you need it to be database-agnostic?

Cheers

@jacobg
Copy link
Author

jacobg commented Sep 11, 2019

I mean just the type class. It would be useful in a common case to just have the type define the id as a string or a number, so that the type class is an abstraction. That way classes outside the data access layer don't need to have a direct dependency on the datastore library.

@sebelga
Copy link
Owner

sebelga commented Sep 12, 2019

Would something like this work (haven't tested it)?

// Database agnostic
interface MyEntity {
  name: string;
  age: number
}

// Union with the specific for the Datastore
const schema = new gstore.Schema<MyEntity & { parentField: entity.Key }>({ ... })

@jacobg
Copy link
Author

jacobg commented Sep 12, 2019

But I'd like MyEntity to have a reference to the parent field, but without the entity.Key type, perhaps as a long or some opaque string-representation of the full key.

@sebelga
Copy link
Owner

sebelga commented Sep 12, 2019

I am not sure I fully understand your goal.

The required type for the parentField property is a entity.Key. Nothing stops you to set it as any if you want to hide the implementation behind. But then, you won't get an error if you don't provide the correct type.

@sebelga
Copy link
Owner

sebelga commented Sep 16, 2019

Sorry, I think I finally understand what you mean. You are saying that in order to set the Type we need to import in a subfolder of the google-cloud/datastore lib?

import { entity } from '@google-cloud/datastore/build/src/entity';

This is indeed not good and I will open a PR on Google repo when I finish converting the code to Typescript

@jacobg
Copy link
Author

jacobg commented Sep 17, 2019

There's the dependency issue, and there's also just having to use the datastore library's entity key in a type, where it would be nice to just use a string or number, and have the schema convert it to the native type.

@sebelga
Copy link
Owner

sebelga commented Sep 17, 2019

Can you detail how exactly you would see that?

So you will be providing an entity Key when saving (an object with an id, a namespace and possible ancestors). How is Typescript going to give you validation error if you type it as number?

The Type you provide to your schema is a generic Typescript that is being forwarded and consumed later.

const addressKey = AddressModel.key('abc'); // This is typed as an entity.Key

// Here you can't pass a number or TS will complain 
const user = new User({ address: addressKey }); 

Sorry, I would need to see an example of code to understand how you would do it. Thanks!

@jacobg
Copy link
Author

jacobg commented Sep 17, 2019

Sure!

Here's an example set of schemas:

  const OrganizationSchema = new gstore.Schema<types.Organization>({
    name: { type: String, required: true }
  })

  const OrganizationModel = gstore.model<types.Organization>(
    'Organization', OrganizationSchema)

  const EmployeeSchema = new gstore.Schema<types.Employee>({
    name: { type: String, required: true },
    organization: { type: gstore.Schema.Types.Key, ref: OrganizationModel.entityKind, required: true }
  })

For this Employee type, I have to declare:

  export type Employee = {
    name: string
    organization: entity.Key
  }

Whereas I'd prefer to declare my type like this:

  export type Employee = {
    name: string
    organization: string | number
  }

That way, this type can be used outside the data access layer, and no other parts of the application need to know or care that the data access layer uses Cloud Datastore.

There still may need to be an extra type declared that gets joined with each specific type which includes the primary key, but it can be database agnostic:

export type SavedType<T> = {id: string | number} & T;

Does that clarify?

@sebelga
Copy link
Owner

sebelga commented Sep 18, 2019

Thanks for the example, I will look into it after I finish refactoring the code base to Typescript.

You haven't shown me how you would save a Key as organisation when creating a new Employee. string | number is not enough, there is a possible namespace and ancestor path to provide.

@jacobg
Copy link
Author

jacobg commented Sep 18, 2019

Thanks!

Perhaps the entity key could be converted to an opaque string, rather than an object? It's convention for most databases to have some sort of reference key, either a number or a string.

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

No branches or pull requests

2 participants