IjzerenHein / firestorter

Use Google Firestore in React with zero effort, using MobX 🤘
http://firestorter.com
MIT License
378 stars 50 forks source link

firestorter/web supports only cjs builds #184

Open dearlordylord opened 1 year ago

dearlordylord commented 1 year ago

Hi, I found a weird issue on esm project.

firestorter/web uses only cjs build of firestore it seems

as a result, in esm project firestore compares firestore with firestore and finds that firestore is not firestore because firestore import is esm2017 in our case; there are just two same definitions of firestore that doesn't match when firebase lib code compares them with instanceof and it causes it to throw Expected first argument to collection() to be a CollectionReference, a DocumentReference or FirebaseFirestore even when the FirebaseFirestore is passed properly

the issue is that it doesn't understand that FirebaseFirestore from cjs and FirebaseFirestore from esm2017 builds are the same thing (because of instanceof)

the specific example of this is collection: (path: string) => collection(firestore, path), in firestorter/web where firestore provided is an import from esm2017 build, and collection comes from cjs build, then it internally checks firestore with t instanceof hh

when I copy-paste the firestorter/web function into my app, it starts using node_modules/firebase/node_modules/@firebase/firestore/dist/index.esm2017.js again and definitions of class hh { matches.

References: hh/vh classes mentioned are minimized https://github.com/firebase/firebase-js-sdk/blob/aea4a4471306b089a02b207d2df285dfc71666c7/packages/firestore/src/api/database.ts#L91 (I debugged this in browser, therefore shortened names from dists of Firebase js SDK)

and the comparison takes place in https://github.com/firebase/firebase-js-sdk/blob/aea4a4471306b089a02b207d2df285dfc71666c7/packages/firestore/src/lite-api/reference.ts#L406 (don't mind 'lite-api', I think they extend it down the line, I checked specifically that it works with the full API and that I don't use /lite anywhere)

I think providing two builds for firestorter/web like you have provided for root firestorter could fix this; I may make a minimal reproduction at some point also, if needed

IjzerenHein commented 1 year ago

Hmm interesting, not sure what's going on here

Seems like some kind of bundling issue indeed.

Just curious, what if you were to copy the contents of web/index.ts into your own project and use that to make a context and initialize firestorter with it. Does that work?

dearlordylord commented 1 year ago

@IjzerenHein Yes it does, I actually walk around it like this for now. My project uses the classes from Firebase esm2017 so after doing so it all matches well

IjzerenHein commented 1 year ago

Alright, well that’s good to hear. I don’t really know what to do with this issue at this time, since the problem is so intertwined with your bundler. What bundles/stack are you using? I think the only way I can help you, is when you provide a minimal reproducable repo that I can test with

dearlordylord commented 1 year ago

Hey, just a notification that I'm still experiencing this issue and would like to provide you with minimal repro at some point. No time right now but I remember. Nx is what I use to build. The specific configuration uses nextjs underneath.