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

id field value is override by vuefire #1472

Closed
tlserver opened this issue Dec 4, 2023 · 3 comments
Closed

id field value is override by vuefire #1472

tlserver opened this issue Dec 4, 2023 · 3 comments

Comments

@tlserver
Copy link
Contributor

tlserver commented Dec 4, 2023

Reproduction

Please see step 2

Steps to reproduce the bug

  1. Create a document in firestore that contain a field named id such as
{
    "id": "123",
    "name": "test document"
}
  1. Read the document via vuefire and print it out
import {doc, getFirestore} from 'firebase/firestore';

const firestore = getFirestore();
const docStm = useDocument(doc(firestore, 'AnyCol/AnyDoc'));
watch(docStm, console.log, {immediate: true});
  1. Check the console
{
    "id": "AnyDoc",
    "name": "test document"
}

Expected behavior

id should be 123

Actual behavior

id is overrided to document ID (AnyDoc)

Additional information

It is good for vuefire to provide access to document id but I think it should not use a normal field name id, since it is a valid field name for firestore too. Use a special field name such as .id can avoid conflict with document data.
One constraint about the firestore document field name is 'not start with a dot(.)'.
image

Also, as suggested in #180, I think the following properties should be exposed to the vuefire data object as non-enumerable properties:

  1. the snapshot itself (optional, better if provided)
  2. all properties(id, metadata, ref) of document snapshot
  3. some properties(metadata, queery) of query snapshot
Copy link
Member

posva commented Dec 4, 2023

The id property is needed for VueFire to apply certain optimizations, as noted in documentation. You will need to name your property something else. Adding a dot in front makes it annoying to use object['.id'] in v-for loops and db writes that's why it wasn't used.

Regarding other properties, that same page also explains how to add custom data to the document. Adding everything by default would bloat the data, especially in SSR.

@posva posva closed this as not planned Won't fix, can't repro, duplicate, stale Dec 4, 2023
tlserver added a commit to tlserver/vuefire that referenced this issue Dec 4, 2023
@tlserver
Copy link
Contributor Author

tlserver commented Dec 4, 2023

Yes, it may be annoying, but it at least does not break things which is more importance.

You will need to name your property something else.

If this issue is not planned to be fixed, this should be stated in document with highlighting.

But I think, Vuefire should use .id internally, user can still define a custom converter to add a id field back to the object and keep v-for clean. So that, .id is not annoying any more. BUT, now we have NO WAY to get the real id field in document even adding custom converter since vuefire is not working properly with a "wrong" id.


Since useDocument() return VueFirestoreDocumentData, defining a global firestore converter does not make typescript happy when reading other field. Type casting or bypassing the TS rule every where is needed, it is annoying.
How about just add ref by default? If it is still too big to have a DocumentReference object, a full path string is still very useful for everyone.

I really hope this issue be fixed, this commit may help.

Copy link
Member

posva commented Dec 4, 2023

For the moment, id is a very practical default and will likely stay that way. What is actionable in the near future is allowing overwriting the global type so one can customize the global converters while things being typed and of course document it with some practical examples like adding the ref like you propose

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

No branches or pull requests

2 participants