deepkit / deepkit-framework

A new full-featured and high-performance TypeScript framework
https://deepkit.io/
MIT License
3.24k stars 124 forks source link

Firestore mapper - expected (plain | mongo) in argument? #12

Closed otri closed 1 year ago

otri commented 4 years ago

I'm trying to build a classToFirestore and firestoreToClass mapping, however the call for deleteExcludedPropertiesFor only takes "mongo" or "plain" target, and this constraint extends to exclude. So I'm not sure how best to wrangle this, other than to drop the deleteExcludedPropertiesFor, which would mean all the excluded fields would be retained.

Or forking the whole project?

Advice would be really appreciated!

marcj commented 4 years ago

Let's rename it to 'database'. That fits nicely in the refactoring soon where marshal-mongo will be renamed to marshal-database, where we then have multiple adapters (mongo, mysql, firebase,...) with ORM capabilities. Maybe it's a good time to do this work now, where we have our first third-party serialization target. Do you have a big mapping list? In mongo its only a couple of fields. I ask because ideally we have JIT'ed serialization functions, which makes it necessary to register mapping functions per type per direction (eg 'date':class->firebase)

marcj commented 4 years ago

Ok, I'm in the middle of refactoring that stuff and implemented a JIT engine that gave additional up to 15x (classToPlain even 50x) performance improvements and made writing adapters easier. Let me get that done tonight or tomorrow, then I come back to you with documentation on how you can integrate Firebase as easy as possible.

otri commented 4 years ago

That's awesome. Thank you!

On Sun, Feb 2, 2020 at 5:38 Marc J. Schmidt notifications@github.com wrote:

Ok, I'm in the middle of refactoring that stuff and implemented a JIT engine that gave additional up to 15x performance improvements and made writing adapters easier. Let me get that done tonight or tomorrow, then I come back to you with documentation on how you can integrate Firebase as easy as possible.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/marcj/marshal.ts/issues/12?email_source=notifications&email_token=AAHHYAWW5OW4XUF4QWHLHT3RAXMVPA5CNFSM4KORODLKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKRGDLI#issuecomment-581067181, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHHYARFS2PQYPRYRQ3CUDTRAXMVPANCNFSM4KORODLA .

--


Aaron Hilton Steampunk Digital

mobile: +81 80 3947 6874 <+818039476874> skype: otristudios facetime: aaron@steampunk.digital Monocle 3D Portrait https://goo.gl/w0JV66

marcj commented 4 years ago

It's done, we got massive performance improvements and I decoupled a lot of stuff. I'm actually very happy about that :)

The way you add new serialization targets is now easier as well. You have to write basically for every type that needs to be casted a little compiler template. Here is all what is needed for MongoDB support:

https://github.com/marcj/marshal.ts/blob/master/packages/mongo/src/compiler-templates.ts

See documentation about the used functions here: https://github.com/marcj/marshal.ts/blob/master/packages/core/src/compiler-registry.ts

You see code like

//my-compiler-templates.ts
import {moment, PropertyCompilerSchema, registerConverterCompiler} from '@marcj/marshal';
registerConverterCompiler('class', 'firebase', 'moment', (setter: string, accessor: string, property: PropertyCompilerSchema) => {
    return `${setter} = ${accessor}.toDate();`
});
registerConverterCompiler('firebase', 'class', 'moment', (setter: string, accessor: string, property: PropertyCompilerSchema) => {
    return {
        template: `${setter} = moment(${accessor});`,
        context: {moment}
    };
});

This code registers a new compiler template for serialization from class to firebase for the type moment. Since firebase does not have Moment.js support, you need this compiler as well for example. You need a couple of those, especially for arrayBuffer and all typed arrays. Mongo stores those as Binary, and firebase has probably a custom binary abstraction. See how mongo did it for examples.

For class you can take the default compiler:

//my-compiler-templates.ts
import {compilerConvertClassToX, compilerXToClass} from '@marcj/marshal';
registerConverterCompiler('class', 'firebase', 'class', compilerConvertClassToX('firebase'));
registerConverterCompiler('firebase', 'class', 'class', compilerXToClass('firebase'));

Here's a list of types you probably want to cover, depending on which types the firebase driver supports

In best case you have only 4 compilers registered (2x moment and 2x class).

Please only define registerConverterCompiler('firebase', 'class', type, ... and registerConverterCompiler('class', 'firebase', type, .... plainToFirebase and firebaseToPlain should work through classToFirebase and firebaseToClass. For example how mongo did it:

export function plainToMongo<T>(classType: ClassType<T>, target: { [k: string]: any }): any {
    return classToMongo(classType, plainToClass(classType, target));
}

It's faster to do it so than manually specifying every compiler template possible for plain -> firebase.


You can then provide little wrapper functions around the core to make autoloading of your compiler-templates possible and give users a clear API. Here's how the mongo implementation is doing it:

https://github.com/marcj/marshal.ts/blob/master/packages/marshal-mongo/src/mapping.ts

Your wrapper function would look like these:

//mapper.ts
import './my-compiler-templates.ts';
import {createClassToXFunction, createXToClassFunction} from '@marcj/marshal';

export function firebaseToClass<T>(classType: ClassType<T>, record: any, parents?: any[]): T {
    return createXToClassFunction(classType, 'firebase')(record, parents);
}

export function classToFirebase<T>(classType: ClassType<T>, instance: T): any {
    if (!(instance instanceof classType)) {
        throw new Error(`Could not classToFirebase since target is not a class instance of ${getClassName(classType)}`);
    }
    return createClassToXFunction(classType, 'firebase')(instance);
}
otri commented 4 years ago

Fantastic, nicely done. Performance is already spectacular, so this beyond cool. I'll be diving into the classToFirebase and firebaseToClass implementation this week. Really appreciate the detail you went into for framing out the firebase adapter. It only has to adapt a couple data types, Timestamp and Binary. Everything else is pretty much plain JSON for documents.

However, I'm not sure how I would handle sub-collections though. That's a Firestore specific feature that might not have any good direct mapping. And I'm not sure I'll have the time to go deep on sub-collections support. Our needs for Firestore are pretty limited.

Oh, just realized Firebase Database and Firestore are close but different databases. Might need to create a "classToFirestore" specific variant. I'll know more when I dig into it.

leandro-manifesto commented 4 years ago

Hey guys, I'm toying around with making a Firestore adapter as well, but in my case I want the Timestamp values to be converted to luxon's DateTime.

import { Timestamp } from '@google-cloud/firestore';

import { DateTime } from 'luxon';

import { ClassType } from '@marcj/estdlib';
import {
    compilerConvertClassToX,
    compilerXToClass,
    registerConverterCompiler,
} from '@marcj/marshal';

const compilerClassToFirestore = compilerConvertClassToX('firestore');

registerConverterCompiler('class', 'firestore', 'class', (setter, accessor, property, reserveVariable) => {
    if (property.resolveClassType === DateTime) {
        return {
            template: `${setter} = Timestamp.fromMillis(${accessor}.toMillis());`,
            context: {
                Timestamp,
            },
        };
    }

    return compilerClassToFirestore(setter, accessor, property, reserveVariable);
});

const compilerFirestoreToClass = compilerXToClass('firestore');

registerConverterCompiler('firestore', 'class', 'class', (setter, accessor, property, reserveVariable) => {
    if (property.resolveClassType === DateTime) {
        return {
            template: `${setter} = DateTime.fromMillis(${accessor}.toMillis());`,
            context: {
                DateTime,
            },
        };
    }

    return compilerFirestoreToClass(setter, accessor, property, reserveVariable);
});

Is this a proper way to do it? Also, is it necessary to register converters for the basic types (boolean, number, string, array and object) as well?

marcj commented 4 years ago

@leandro-manifesto sorry, forgot to answer you.

Is this a proper way to do it?

Yes, you can do it like that.

Also, is it necessary to register converters for the basic types (boolean, number, string, array and object) as well?

Only if the firestore driver itself returns different types and that need to be translated to JS types. I guess the driver returns boolean already as Boolean, number as Number etc, so nothing to do here.

lionelhorn commented 3 years ago

As the api seems to have changed a bit since marshal, how could I achieve the following with @deepkit/type:

Let's say I have the following type

class Lifecycle {
  @t public uid: string;
  @t.type(firestore.Timestamp) public estimatedPickupAt?: firestore.Timestamp;
  @t.type(firestore.Timestamp) public acceptedAt?: firestore.Timestamp;
}

And a typed instance with this data:

{   
  uid: "test",
  estimatedPickupAt: {
    seconds: 1610301294,
    nanoseconds: 0,
  },
  acceptedAt: {
    seconds: 1610299624,
    nanoseconds: 0,
  }
}

Timestamp is a type from the firestore web sdk. When I want to save my data to firestore, I need to preserve the Timestamp object so the firestore sdk properly stores it.

Currently, when using classToPlain(Lifecycle, obj), it removes the types of estimatedPickupAt and acceptedAt.

How can I filter out the properties I want @deepkit/type to not serialize?

I've seen your recent commits related to @t.serialize https://github.com/deepkit/deepkit-framework/commit/1c9094844821d9eb049332a83c69e80347f5472a Will that help regarding my use case?

marcj commented 1 year ago

I think this is very outdated and not needed anymore. I'm going to close this, but feel free to reopen if it is still relevant.