Skip to content

[sub-PR] Constrain useFirestoreConnect query types to enforce our firestore query patterns#1076

Merged
mike-lvov merged 1 commit into
devalias-fix-cadams-doc-or-collection-queriesfrom
devalias/fix-pr-801-type-discriminators
Jan 14, 2021
Merged

[sub-PR] Constrain useFirestoreConnect query types to enforce our firestore query patterns#1076
mike-lvov merged 1 commit into
devalias-fix-cadams-doc-or-collection-queriesfrom
devalias/fix-pr-801-type-discriminators

Conversation

@0xdevalias

@0xdevalias 0xdevalias commented Jan 14, 2021

Copy link
Copy Markdown
Contributor

Sub-PR to #801 that constrains the useFirestoreConnect query types to enforce our firestore query patterns.

This reinstates and improves upon the original type narrowing intended by #784, as called out in #801 (comment)

@0xdevalias 0xdevalias added the 💪 enhancement Enhancements/improvements to existing functionality label Jan 14, 2021
@0xdevalias 0xdevalias requested review from a team and mike-lvov January 14, 2021 06:05
@0xdevalias 0xdevalias self-assigned this Jan 14, 2021
@0xdevalias

0xdevalias commented Jan 14, 2021

Copy link
Copy Markdown
Contributor Author

If you want to play around with this in your IDE to see the type correctness, these are the 'manual test cases' I was using:

const foo1: SparkleRFCollectionQuery = {
  collection: "users",
};

const foo1Invalid: SparkleRFCollectionQuery = {
  collection: "users",
  storeAs: "users", // not allowed to use the name of a root collection in storeAs
};

const foo2: SparkleRFCollectionWhereQuery = {
  collection: "users",
  where: ["some id", "in", ["a"]],
  storeAs: "chatUsers",
};

const foo2Invalid: SparkleRFCollectionWhereQuery = {
  collection: "users",
  doc: "aaa", // doc not allowed for this type
  where: ["some id", "in", ["a"]],
  storeAs: "chatUsers",
};

const foo3: SparkleRFSingleDocumentQuery = {
  collection: "users",
  doc: "foo",
  storeAs: "chatUsers",
};

const foo3Invalid1: SparkleRFSingleDocumentQuery = {
  collection: "users",
  doc: "foo",
  where: ["some id", "in", []], // where not allowed in this type
  storeAs: "chatUsers",
};

const foo3Invalid2: SparkleRFSingleDocumentQuery = {
  collection: "users",
  doc: "foo",
  subcollections: [], // subcollections not allowed in this type
  storeAs: "chatUsers",
};

const foo4: SparkleRFSubcollectionQuery = {
  collection: "users",
  doc: "foo",
  subcollections: [],
  storeAs: "chatUsers",
};

const foo4a: SparkleRFSubcollectionQuery = {
  collection: "users",
  doc: "foo",
  subcollections: [],
  where: ["some id", "in", []],
  storeAs: "chatUsers",
};

const foo4Invalid: SparkleRFSubcollectionQuery = {
  collection: "users",
  // missing required doc
  subcollections: [],
  storeAs: "chatUsers",
};

// Note: this is just here to remove any 'variable is not used' errors
console.log(
  foo1,
  foo1Invalid,
  foo2,
  foo2Invalid,
  foo3,
  foo3Invalid1,
  foo3Invalid2,
  foo4,
  foo4a
);

Depending on how your IDE surfaces type errors, you'll see something like the following when hovering over one of the invalid configs:
image

Comment thread src/hooks/useVenueId.ts
Comment on lines 7 to +9
const WelcomePage: React.FunctionComponent = () => {
// @dept Probably we should remove this piece of software if it is not functional anymore
useFirestoreConnect("marketingemails" as ValidFirestoreKeys);
// @debt we should remove this code file if it's not being used anymore
useFirestoreConnect("marketingemails" as ValidFirestoreRootCollections);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[OOS] We should remove this code file if it's not being used anymore

@mike-lvov mike-lvov merged commit c9f7c8c into devalias-fix-cadams-doc-or-collection-queries Jan 14, 2021
@mike-lvov mike-lvov deleted the devalias/fix-pr-801-type-discriminators branch January 14, 2021 09:26
mike-lvov added a commit that referenced this pull request Jan 14, 2021
…FirestoreConnect import (#801)

* Undo "Revert "Prevent pathological queries when doc is not specified (#784)" (#797)"

* Replace useParam usage with useVenueId across the codebase

* constrain useFirestoreConnect query types to enforce our firestore query patterns + update related usages (#1076)

Co-authored-by: Chris Adams <chris@cadams.com.au>
Co-authored-by: Mike Lvov <mike.lvov.soft@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

💪 enhancement Enhancements/improvements to existing functionality 💥 tech-debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants