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

Referenced documents do not have id #1263

Closed
donatas-luciunas opened this issue Dec 20, 2022 · 13 comments
Closed

Referenced documents do not have id #1263

donatas-luciunas opened this issue Dec 20, 2022 · 13 comments
Labels
bug firestore new Cloud Store

Comments

@donatas-luciunas
Copy link

donatas-luciunas commented Dec 20, 2022

Reproduction

N / A

Steps to reproduce the bug

Use useDocument for a document that has at least one reference

Expected behavior

Referenced document has id property

Actual behavior

Referenced document does not have id property

Additional information

I guess the fix should be somewhere here

const [data, refs] = extractRefs(

A default converter should be added for refs here.

@donatas-luciunas donatas-luciunas changed the title No converter is used for refs Referenced documents do not have id Dec 20, 2022
@posva posva added feature request firestore new Cloud Store labels Dec 21, 2022 — with Volta.net
@michaelpelletier
Copy link

Thanks for posting this! I'd posted on the Vue Discord last night hoping to get an answer as to what I was doing wrong that useDocument was giving me the content of the referenced items but not the actual ID.

@posva
Copy link
Member

posva commented Dec 21, 2022

Note that if you only want the id, you can use maxRefDepth: 0

@michaelpelletier
Copy link

Yeah, I want both, though. The use case I was describing on the Discord is something like this:

I have a student who has a collection of current courses that they are enrolled in this semester. On the student's Dashboad, for each of those courses, I can display the information like the course title or when it meets or who the professor is, but because I can't access the ID, I couldn't build a link to actually view that course's page.

@donatas-luciunas
Copy link
Author

I recommend to fix it this way
This line

refs[refSubKey] = ref
should be changed to

refs[refSubKey] = ref.withConverter(firestoreDefaultConverter);

Copy link
Member

posva commented Dec 22, 2022

yeah, that would be one way. I think we could also have an option for this that allows to use any custom converter and that by default uses the default converter. What do you think?

@donatas-luciunas
Copy link
Author

donatas-luciunas commented Dec 22, 2022

Sorry, I mean DEFAULT_OPTIONS.converter (source) instead of firestoreDefaultConverter.
This way as you say a developer could provide a custom converter.

Providing custom converter per dependency could also be nice. In my head a solution could be additional useDocument options property mask that follows retrieved document structure while also providing instructions how to expand it, for example

{
  "converter": commentConverter,
  "$user": {
    "converter": userConverter,
    "$roles": {}  // instructs to expand with default converter
  }
}

Not mentioned fields should not be expanded. I use $ prefix for actual document fields.
This would also solve #281
This seems to me as a separate feature.

Copy link
Member

posva commented Dec 22, 2022

The converter will likely be passed as an argument to be able to use local options.

I think that instead, allowing both the converter object and a function that returns a converter based on the ref or false or null to skip it could also enable this behavior in a more concise syntax:

useDocument(..., {
  refConverter: customConverter
})

useDocument(..., {
  refConverter: (docRef) => docRef.id.includes(...) ? false : customConverter
})

@donatas-luciunas
Copy link
Author

Sure, that could work as well.

Could you please implement the use of globalFirestoreOptions.converter separately? This seems to be a quick fix. This is a blocker for me to migrate from v2 to v3 😬

Copy link
Member

posva commented Dec 22, 2022

Unfortunately it will have to wait a bit. Use https://www.npmjs.com/package/patch-package and locally patch the package in the meantime, it should be pretty straightforward. v2 didn't have this behavior either

@donatas-luciunas
Copy link
Author

Thanks for recommendation.

I use 2.2.5 and it gives id property on referenced document.

@michaelpelletier
Copy link

Just popping back to say patch-package with donatas-luciunas's original suggestion worked like a dream.

@posva posva added bug and removed feature request labels Jan 6, 2023 — with Volta.net
@posva posva closed this as completed in fe78629 Jan 6, 2023
@digitalkaoz
Copy link

hey @posva or @donatas-luciunas i just popped in with the same problem, i simply cant get the id in my resolved reference:

const coll = collection("projects").withConverter(globalFirestoreOptions.converter)

should be enough i guess, to have all subsequent calls to useDocument or useCollection resolve the ids as long as they are using the coll above?!

what am i missing here?

@digitalkaoz
Copy link

nvm, i found it (as it says in the doc, the id property is not enumerable)

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

No branches or pull requests

4 participants