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

What is the right way to declare type for data object, passed to updateDoc? #89

Closed
mirik-k opened this issue Jan 26, 2023 · 7 comments
Closed
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers

Comments

@mirik-k
Copy link

mirik-k commented Jan 26, 2023

Hello, firstly, thank you for amazing library that makes everything easier!!!

I have some trouble:

I need to implement a helper function, that makes it possible to not add some required parameters in every call of updateDoc/batch.update, and I'm unable to figure out, what type of FirestoreUpdateParams.data parameter should be to pass the typescript validation, and, more importantly, throw errors, if I'm passing wrong data to this helper function.

Here is the code:

import { DocumentData } from 'firebase/firestore';
import { serverTimestamp, updateDoc, DocumentReference } from 'firelordjs';

import type { WriteBatch } from 'firelordjs/dist/types';
import { ChatsMetaType } from '~/models/firestore/chat';

import store from '~/store';
import type { UpdateAction } from '~/types/global';

const autoLog = true;

const timestamp = serverTimestamp();

type FirestoreUpdateParams = {
  batch?: WriteBatch;
  ref: DocumentReference<ChatsMetaType>;
  data: __HERE__;
  action: string;
  params?: DocumentData;
  log?: boolean;
};

export function $firestoreUpdate({
  batch,
  ref,
  data,
  action,
  params = {},
  log = autoLog,
}: FirestoreUpdateParams): Promise<void> {
  const mixin = {
    update_action: {
      type: action,
      params,
      timestamp,
    } as UpdateAction,
    updated_by_uuid: store.getters['auth/user'].uid as string,
    updated_at: timestamp,
    updated_by_platform: 'web' as string,
  };

  const mixedData = {
    ...data,
    ...mixin,
  };

  if (batch) {
    if (log) {
      console.log(
        '$firestoreUpdate in batch',
        ref.path,
        action,
        params,
        mixedData,
      );
    }
    batch.update(ref, mixedData);
    return Promise.resolve();
  }

  if (log) {
    console.log('$firestoreUpdate', ref.path, action, params, mixedData);
  }
  return updateDoc(ref, mixedData);
}

Expecting to use it like this:

firestoreUpdate({
  ref: ...,
  data:  {
     name: 'string', // Ok, no error, name is defined in MetaType
     name: 0,  // Error: type must be string
     unknownProp: 'value', // Error: no such property
  },
  ....
});

But this type would be also useful when creating data object in few places step by step

const dataObject: __HERE__ = {};

dataObject.name = 'string'; // No error 
dataObject.name = 0; // Error, must be string
dataObject.unknownProp = 'value'; // Error, unknown prop

// ...

firestoreUpdate({
  ref: ...,
  data: dataObject,
  ....
});
@mirik-k mirik-k added the enhancement New feature or request label Jan 26, 2023
@tylim88
Copy link
Owner

tylim88 commented Jan 26, 2023

By design Firelord enforce common best practice, one of the practice is no optional because of this reason

I strongly recommend that during MetaType creation, use default value of other type

type __HERE__ = {name: string | false, age: number | false }
type ABC = MetaTypeCreator< __HERE__ , "abc">

const dataObject: __HERE__ = { name: false, age: false};

false is an ideal default value,

I do not recommend null because of it complicated query behavior

undefined on the other hand is not a valid Firestore write type (Firestore cannot store undefined)

there is even a setting to ban the use of null type in MetaTypeCreator (I forgot to document it, will update the doc soon), it is called banNull

@tylim88
Copy link
Owner

tylim88 commented Jan 26, 2023

optional is one of the highly requested features because it is very common Typescript feature

Firelord does not support optional to prevent user from accidentally cripple their query capabilities in long term

This of course make some developers feel uncomfortable

Currently I am brainstorming whether we can achieve a middle ground in V3, but that is very unlikely because Firelord aim to be the safest wrapper(but will still allow optional in update operation), I will also probably ban the use of null altogether in V3

hopefully in V3 update ca accept something like type __HERE__ = {name?: string, age?: number }

@tylim88 tylim88 added documentation Improvements or additions to documentation good first issue Good for newcomers question Further information is requested labels Jan 26, 2023
@tylim88
Copy link
Owner

tylim88 commented Jan 26, 2023

after giving it a deeper thought, my suggested solution does not work

I need time to patch up a solution

meanwhile you can use type __HERE__ = {name?: string, age?: number } and suppress the type error, you dont need to change the meta type

@mirik-k
Copy link
Author

mirik-k commented Jan 26, 2023

Thanks for the response, but I'm not talking about optional props, all props in my MetaType are not optional, but i want to have a Type, that have one or more of those all props. Here are more parts of code to make clarify my issue:

MetaType Creation:

type FirestoreChatData = {
  _uid: string;
  name: string;
  //
  update_action: UpdateAction;
  //
  created_at: ServerTimestamp;
  creator_uuid: string;
  created_by_platform: string;
  //
  updated_at: ServerTimestamp;
  updated_by_uuid: string;
  updated_by_platform: string;
  //
  deleted_at: ServerTimestamp | null;
  deleted_by_uuid: string | null;
  deleted_by_platform: string | null;
};

export type ChatsMetaType = MetaTypeCreator<FirestoreChatData, 'Chats'>;

Then this works

updateDoc(ref, {
  name: 'New Name',
});

But this doesn't, I've expected, that Partial will do the job, but updateDoc doesn't accept that

const data: Partial<ChatMetaType['write']> = {
  name: 'New Name',
};
updateDoc(ref, data)

About undefined props: why just not drop them when updating doc?

@tylim88
Copy link
Owner

tylim88 commented Jan 26, 2023

please do npm i firelordjs@2.2.0 (or yarn) then you can use the partial type

release note: https://github.com/tylim88/FirelordJS/releases/tag/2.2.0

@tylim88
Copy link
Owner

tylim88 commented Jan 26, 2023

you must turn on exactOptionalPropertyTypes in tsconfig

About undefined props: why just not drop them when updating doc?

they are needed to explain stuff before the era of exactOptionalPropertyTypes

@mirik-k
Copy link
Author

mirik-k commented Jan 27, 2023

Thanks for quick resolving!! And for exposing types too, wanted to ask about it, but forgot))

@mirik-k mirik-k closed this as completed Jan 27, 2023
@tylim88 tylim88 removed the question Further information is requested label Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants