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

Refactoring to version 9 #260

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

amilosmanli
Copy link
Contributor

Refactored code to firebase v9 modular

  • Fixes issues with total item count in lazy mode

This is probably a breaking change

@amilosmanli
Copy link
Contributor Author

@benwinding took me some time to clean up the branch.

Most of the changes are in yarn.lock file, main code changes aren't that big

@matthieup240
Copy link

matthieup240 commented Feb 21, 2023

Hi, will it be merged soon ?

package.json Outdated
"firebase-tools": "11.x",
"gulp": "^4.0.2",
"jest": "^23.6.0",
"microbundle": "^0.15.0",
"prettier": "^2.8.3",
"prettier-plugin-organize-imports": "^3.2.2",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prettier package is an aesthetic choice, it shouldn't be included in this PR, as there's tonnes of import changes that have no effect on the behaviour of the system.

Mixing linting & functional changes in the same PR's makes the git history confusing and hard to see exactly what changes something.

I'll allow this, but next time, please restrict PR's to the smallest change necessary to make review easier. Linting PR's on the other hand, can be huge as they're not functional changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benwinding I reverted all the prettier and tslint changes, but this one slipped in somehow.

Let me know if you want me to revert this back.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that would be great actually 🙏 Since this is a major firebase upgrade 👍

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just make another PR with the prettier-plugin-organize-imports linting changes, that can get merged in before (so these changes will be reduced)

@matthieup240
Copy link

I have a question about the collectionQuery with this new firebase version 9.

The usually c.where('__name__', 'in', users) syntax will not work.

How to do the equivalent ?

I found this in the Firestore doc :
collectionQuery: c => query(c, where('__name__', 'in', users)

This mean we have to import the query from the 'firebase/firestore' plugin ?

@amilosmanli amilosmanli force-pushed the refactoring-to-version-9 branch from ae72d63 to beba28b Compare March 11, 2023 18:10
@amilosmanli
Copy link
Contributor Author

amilosmanli commented Mar 11, 2023

@benwinding, I updated the PR. PTAL

Copy link
Owner

@benwinding benwinding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this huge effort @amilosmanli !! 👏
Much easier to see the changes, I'll merge this and deploy a newer minor release soon 👌

total = (page - 1) * perPage + data.length;
log('apiGetListLazy', { message: `It's last page of collection.` });
}
let total = await getCountFromServer(noPagination);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! I didn't know they added getCountFromServer 👏 🎉

Comment on lines -126 to -131
public auth(): FireAuth {
return this.app.auth();
}
/** @deprecated */
public storage(): FireStorage {
return this.app.storage();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still keep these @deprecated fields, if we remove them, then people's code might break in weird ways

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll re-add them, before the deploy 👌

@benwinding benwinding merged commit 1a03111 into benwinding:master Mar 14, 2023
@benwinding
Copy link
Owner

Deployed as [email protected] let me know how it goes! 🎉

@amilosmanli
Copy link
Contributor Author

@benwinding thank you very much for accepting the changes!

let me know if there are any bug reports for 4.1.0. Will be happy to help to resolve them

@ucefkh
Copy link

ucefkh commented Apr 21, 2023

Hey @amilosmanli I have this issue which normally should have been fixed by your changes! (and using getCountFromServer )

#272

Thanks for the hard work!

@CBrian04
Copy link

CBrian04 commented Sep 28, 2023

Hi everybody!
Migrating collectionQuery from c.where() to query(c, where()) I got a firebase error saying
FirebaseError: [code=invalid-argument]: Type does not match the expected instance. Did you pass a reference from a different Firestore SDK?
Does someone had been confronted to that issue ?

filters={{ collectionQuery: (c: FireStoreCollectionRef) => query(c, where("status", "!=", "checkout")) }}

@MykolaRomaniv
Copy link

Hi everybody! Migrating collectionQuery from c.where() to query(c, where()) I got a firebase error saying FirebaseError: [code=invalid-argument]: Type does not match the expected instance. Did you pass a reference from a different Firestore SDK? Does someone had been confronted to that issue ?

filters={{ collectionQuery: (c: FireStoreCollectionRef) => query(c, where("status", "!=", "checkout")) }}

Same error. Do you manage to fix it?

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

Successfully merging this pull request may close these issues.

7 participants