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

[BUG] Mapped type becomes undefined #20

Closed
mirik-k opened this issue May 12, 2023 · 11 comments
Closed

[BUG] Mapped type becomes undefined #20

mirik-k opened this issue May 12, 2023 · 11 comments
Labels
enhancement New feature or request

Comments

@mirik-k
Copy link

mirik-k commented May 12, 2023

Hi there, I have some trouble using this library in my project. This was working in firelordjs@<=2.4, don't know if it works in firelordjs@>=2.5, didn't check yet

  1. Version?
    2.4.15
  2. What is the Meta Type?
import type { MetaTypeCreator } from 'firelord';

type SomeObject = {
  key: string;
};

type FirestoreDummyData = {
  string: string; // ok
  array: string[]; // ok
  number: number; // ok
  regular_object: {
    test: string;
  }; // ok
  mapped_primitive: Record<string, string>; // becomes undefined
  mapped_object: Record<string, SomeObject>; // becomes undefined too
};

export type DummyMetaType = MetaTypeCreator<FirestoreDummyData, 'Dummy'>;

const test: DummyMetaType['read'] = {
  string: 'test', // ok
  array: ['test'], // ok
  number: 10, // ok
  regular_object: { test: 'test' }, // ok
  mapped_primitive: { // TS2322: Type '{ test: string; }' is not assignable to type 'undefined'.
    test: 'test',
  },
  mapped_object: {  // TS2322: Type '{ test: { key: string; }; }' is not assignable to type 'undefined'.
    test: {
      key: 'test',
    },
  },
};
  1. What operations are you trying to run?
    I'm trying to use mapped object in meta type
  2. What do you expect?
    Mapped type should work as intended
  3. What is actually happening?
    Mapped type becomes undefined in resulted MetaType['read']
@mirik-k mirik-k added the bug Something isn't working label May 12, 2023
@tylim88
Copy link
Owner

tylim88 commented May 12, 2023

This is part of design

image

https://github.com/tylim88/FirelordJS#dropped-to-do

@mirik-k
Copy link
Author

mirik-k commented May 12, 2023

That's very sad, since it makes this library unusable for our project, and it was almost fully integrated to it, so we'd probably will need to rewrite all the code again(((
The data, that is stored in that objects is not supposed to be queried
In the other hand - there are still ways to exceed 1MB limit - so you can't avoid that only by not allowing to use mapped object
Anyway, it worked before, if it is possible, maybe just do some config to make it possible for those, who need it, and disable it by default?

Thanks in advance

@tylim88
Copy link
Owner

tylim88 commented May 12, 2023

Firelord enforces some practices

one of it is banning mapped type

the biggest concern is, mapped type is very hard to query because you always need to know the value of the key in order to query

example data type:

type Data= Record<string, number>

now imagine you need to query them, how are you going to do it?

where(?, '==', 1)

well, you need to know both key and value, unlike data type like this

type Data= {a: number}

where('a', '==', 1), in this case you always know the key is going to be a

now imagine somebody created data type like this Record<string, Record<string, Record<string, number>>>

He need to know 4 information: 3 keys and 1 value before he can even query the data

maybe your intention is to reduce the cost, but I believe mapped type is a trap that could cripple your query in the future

you might want to convert them into a collection of document instead

option is a good idea, I will see what I can do

@mirik-k
Copy link
Author

mirik-k commented May 12, 2023

I understand why it's banned, but in our case we never use this for any data, that is meant to be queried. The data there is totally dynamic, so we cannot predict. I would even tell you, that we (almost)actually have such mapped object possible in our project as you provided as an example :). In short:

type DummyData = {
  mapped: Record<string, {
    _uid: string;
    created_at: Timestamp;
    updated_at: Timestamp;
    nested: Record<string, {
      status: 'active' | 'deleted',
      created_at: Timestamp;
      updated_at: Timestamp;
    }>
  }>
}

We could use array with objects, but it will be much more inconvenient to get the needed data from the object, because we will need to iterate over all array to search some entry. Updating such data will be also big headache, and I guess it's even not possible to properly check such updates in firestore rules, but for mapped object we've managed to make reliable checks of valid data added/updated in such objects

@mirik-k
Copy link
Author

mirik-k commented May 12, 2023

BTW, I would also like to ask a config option, that allows to disable update behavior, that makes every update flat, since we also have some (not nested)objects, that are dynamic at all, what means Record<string, unknown>;, and when we update it - it should be replaced fully, not merged with previous data.

For example, where there is such object: { key: 'value', key2: 2 }, and I'm trying to update it with { key2: 3, key3: null } - I expect it to be replaced, but instead I get { key: 'value', key2: 2, key3: null }

This feature actually is awesome, but in our case we'd rather turn it off.

For now, in our front-end we're just using original firestore's writeBatch to create a batch, to avoid this behavior

import { writeBatch } from 'firebase/firestore';
import type { WriteBatch } from 'firelordjs';

export const createBatch = () => writeBatch() as WriteBatch;

Now I need to make some code for backend - and I'm not sure if I would be able to do such thing there

@tylim88
Copy link
Owner

tylim88 commented May 12, 2023

I will see what I can do tomorrow

@tylim88 tylim88 added enhancement New feature or request and removed bug Something isn't working labels May 13, 2023
@tylim88
Copy link
Owner

tylim88 commented May 15, 2023

After some considerations, I allowed Record<string, something> type by default.

I think Firelord should bans what is impossible, not what is possible but could goes wrong, unless it requires minimal efforts from users to make things right

Firelord should not interferes too much with how users design their data model

Everything you need is available in v2.4.16

@tylim88 tylim88 closed this as completed May 15, 2023
@mirik-k
Copy link
Author

mirik-k commented May 15, 2023

Great!! Thanks!

@mirik-k
Copy link
Author

mirik-k commented May 15, 2023

Will you update firelordjs as well?

@tylim88
Copy link
Owner

tylim88 commented May 15, 2023 via email

@tylim88
Copy link
Owner

tylim88 commented May 15, 2023

please install admin v2.4.17 and js v2.5.10

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

No branches or pull requests

2 participants