Removes pathological queries; Creates linting rule to prevent raw useFirestoreConnect import by 0xdevalias · Pull Request #801 · sparkletown/sparkle

0xdevalias · 2020-11-25T09:48:18Z

Background Context: The original PR (#784) was deployed in #795, which broke things in production, and was reverted in #797

Thoughts: @denisdimitrov originally had #780 up to solve the core of our priority-prod-fix concerns for an upcoming event. If that solves the immediate need, then maybe we should resurrect that and get it out sooner, which will triage that need, and then we can work on these fixes over a less risky timeframe.

This PR will fix the issues that were breaking the admin page/anything else it broke in production.

…784)" (#797)" This reverts commit a5e6923.

0xdevalias · 2020-11-25T09:52:28Z

@thecadams Created this branch/draft PR to get visibility back on the board for this issue + link up it's context, but absolutely happy for you to run with it and make the fixes/etc from your original PR.

…ries

cadamsdotcom · 2020-11-25T16:37:24Z

@0xdevalias picking this up now

…ries

cadamsdotcom · 2020-12-14T08:58:15Z

Performance TroubleShooting

…ries

…s-doc-or-collection-queries

mike-lvov · 2021-01-13T13:32:22Z

After conducting extensive research, I couldn't find a way to distinguish not passing doc vs passing undefined explicitly

microsoft/TypeScript#13195

I have found a way to solve this issue by replacing all useParams for venueId with useVenueId and return string | null from useVenueId. I suggest completely removing explicit undefined from the codebase as it is an antipattern to my opinion. This PR can be used as an argument to support my opinion

mike-lvov · 2021-01-13T13:46:14Z

RE: #801 (comment)

Partially closes sparkletown/internal-sparkle-issues#2
Not sure if I should implement other recommendations from this ticket,
since we basically gain nothing in terms of linting/etc.
@0xdevalias would be glad to hear your approach on this

@mike-lvov When you say "since we basically gain nothing in terms of linting/etc", why do you feel we "gain nothing" from it? Even if there aren't any issues it would detect currently, the idea of linting is to ensure that undesirable patterns won't be introduced again in future, and to provide guidance to developers who may try to about what our 'correct' way of implementing it is.

As far as implementing the other recommendations on the ticket, see my comments in sparkletown/internal-sparkle-issues#2 (comment) I believe we should be able to clean up all of those aspects as well, and then we can add a few more eslint rules to ban their usage.

I think I conveyed my thoughts in an unclear way. I'm didn't mean that we don't need linting. I meant that replacing import firebase from "firebase/app" with useFirebase would not result in any additional type safety (as it was with useFirestore vs useSparkleFirestore). I think of other issues as low priority clean-ups.

mike-lvov · 2021-01-13T13:51:15Z

@0xdevalias I think the problem we have been experiencing recently is a lot of unrelated changes happening in one PR. To avoid that, I would rather implement https://github.com/sparkletown/internal-sparkle-issues/issues/2 issues in another PR

0xdevalias · 2021-01-13T21:32:58Z

After conducting extensive research, I couldn't find a way to distinguish not passing doc vs passing undefined explicitly

microsoft/TypeScript#13195

I have found a way to solve this issue by replacing all useParams for venueId with useVenueId and return string | null from useVenueId.

I think using useVenueId instead of the 'raw' useParams instances for this pattern is a good idea. I'd like to know more about the arguments behind the string | null before we do that part though.

I suggest completely removing explicit undefined from the codebase as it is an antipattern to my opinion. This PR can be used as an argument to support my opinion

Can you please expand on this, and your reasoning for it? In what cases are you seeing 'explicit undefined' being used, and in what way do you feel like it could be done better? Why do you see null as a better option than it (assuming that is what you are saying here)?

The original PR that introduced this was #784 (which was reverted in #797 as it broke things in production, and then this current PR was bringing it back + fixes)

Unfortunately #784 (like many of our PR's, which is something we need to change) doesn't have a useful description to outline the intended changes within it, so we have to rely on memory/inferring from the code.

Pulling from the review comments on that PR:

#784 (comment)

@0xdevalias What's the intent of SparkleDocRFQSetting here compared to SparkleRFQSetting?

@thecadams It's to ensure that if your intention is to query for a single doc, you only query for that doc.

@0xdevalias From @thecadams on zoom (paraphrased): essentially we had times when a doc that we were supplying was undefined, and so we were loading the entire collection (because it's an optional param in the underlying useFirestoreConnect hook), when our intent was to load a single tem or bailout.

The relevant type-related code is at https://github.com/sparkletown/sparkle/pull/784/files#diff-254a400761084e5ebd89145b80d201db7f0fd1e2dff8684213ee5d1804ee886aR11-R45

export interface SparkleRFQuery extends ReduxFirestoreQuerySetting {
  doc?: never;
  where: WhereOptions | WhereOptions[];
  storeAs?: ValidFirestoreKeys;
}

export interface SparkleRFDocQuery extends ReduxFirestoreQuerySetting {
  doc: string;
  where?: never;
  storeAs?: ValidFirestoreKeys;
}

Inferring from this code (without actually reading deeply/playing with it), I believe the intent is that:

When you pass doc, you can't pass where (intended to access 1 specific document, so no need for filtering)
When you pass where, you can't pass doc (because you aren't accessing a single document, but filtering over a collection of them)

You can also see that useSparkeFirestoreConnect was deleted in this PR, in favour of the more generalised custom useFirestoreConnect implementation:

https://github.com/sparkletown/sparkle/pull/784/files#diff-53c83c5e9a6c0e4d642a0a04aee496008588554b3abebce7a1a14d5aa88bac24

mike-lvov · 2021-01-13T23:07:48Z

Inferring from this code (without actually reading deeply/playing with it), I believe the intent is that:

When you pass doc, you can't pass where (intended to access 1 specific document, so no need for filtering)
When you pass where, you can't pass doc (because you aren't accessing a single document, but filtering over a collection of them)

Even though I support this intent, it didn't seem to work right. The current implementation of this(null) works good because then you try to pass null there it doesn't like it. So a type string | null will not be accepted by TS. That is the main reasoning behind useVenueId change.

0xdevalias · 2021-01-14T02:30:34Z

RE: #801 (comment)

@mike-lvov:
I think I conveyed my thoughts in an unclear way. I'm didn't mean that we don't need linting. I meant that replacing import firebase from "firebase/app" with useFirebase would not result in any additional type safety (as it was with useFirestore vs useSparkleFirestore). I think of other issues as low priority clean-ups.

That makes sense. I agree that they are lower priority. 👍🏻

RE: #801 (comment)

@mike-lvov:
@0xdevalias I think the problem we have been experiencing recently is a lot of unrelated changes happening in one PR. To avoid that, I would rather implement sparkletown/internal-sparkle-issues#2 issues in another PR

I'm happy for that to be how we approach it. 👍🏻

0xdevalias

Includes requested changes (I can't use the official 'request changes' review mode as I opened the original PR)

0xdevalias · 2021-01-14T03:44:07Z

-  const chatQuery = useMemo(() => {
-    if (!hasElements(chatUserIds)) return undefined;
+  const chatQuery = useMemo<SparkleRFQConfig>(() => {
+    if (!hasElements(chatUserIds)) return;


I believe this will have the same code effect (that undefined) will be returned, yet this change makes that less explicitly obvious. What was the intent behind this change?

0xdevalias · 2021-01-14T03:49:25Z

-  const { venueId } = useParams();
+  const venueId = useVenueId();
  const { user } = useUser();
  const [currentTimestamp] = useState(Date.now() / 1000);


[OOS] This is going to set currentTimestamp the first time this hook is called, and then never updated it again I believe, which seems (from it's usage) like it probably isn't the desired intent.

I think in other places where we have wanted this sort of 'only change every x timeframe' pattern we have used an interval or similar to update the useState. That might be a useful solution here as well.

0xdevalias · 2021-01-14T03:51:57Z

-  const siblingVenuesQuery: SparkleRFQConfig | undefined = !!parentId
+  const siblingVenuesQuery: SparkleRFQConfig = !!parentId


Why was undefined removed here?

0xdevalias · 2021-01-14T03:52:03Z

-  const subvenuesQuery: SparkleRFQConfig | undefined = !!venueId
+  const subvenuesQuery: SparkleRFQConfig = !!venueId


Why was undefined removed here?

0xdevalias · 2021-01-14T03:52:40Z


  // Parent Events
-  const parentVenueEventsQuery: SparkleRFQConfig | undefined =
+  const parentVenueEventsQuery =


Why was undefined removed here?

@mike-lvov can you add the explicit typing back here please

0xdevalias · 2021-01-14T04:02:56Z

+import {
+  SparkleRFQConfig,
+  useFirestoreConnect,
+} from "hooks/useFirestoreConnect";
+import { useVenueId } from "hooks/useVenueId";


Sort with other hooks/ imports

0xdevalias · 2021-01-14T04:03:39Z

 import { useSelector } from "hooks/useSelector";
 import VenueEventDetails from "./VenueEventDetails";
-import { useFirestoreConnect } from "react-redux-firebase";
+import { useFirestoreConnect } from "hooks/useFirestoreConnect";


Sort with other hooks/ imports

0xdevalias · 2021-01-14T04:05:45Z

+      if (!venueId) return history.replace("/admin");
+


Why are we redirecting within this function? I assume the intent of this addition was to ensure venueId isn't undefined/similar by the time it gets passed to .doc(). Why wouldn't we just return early like we do with all of our other guard patterns like this?

Suggested change

if (!venueId) return history.replace("/admin");

if (!venueId) return

I see that later on this same history.replace code is used, so that makes sense.

Though it seems we could move this guard outside of the async fetchVenueFromAPI function and just have it in the root of the useEffect?

I think so, but it still would be the same

Thats fine. I think it just more confused me being inside the inner function, rather than the root of the useEffect

0xdevalias · 2021-01-14T04:12:58Z

-  const allValidQueries: SparkleRFQConfig[] = [
+  const allValidQueries: SparkleRFQConfig = [


This feels more confusing than how it used to be.. probably would have named your 'fusion' type as AnySparkleRFQConfig or something specific about the props the function accepts.

Alternatively, given the change has already been done, this would probably be more correct/easy to understand as SparkleRFQConfigObject[]. 'Hiding' the array'ness of it makes it confusing, and I need to go any look up the original 'fusion type' to understand whats going on.

0xdevalias · 2021-01-14T04:15:12Z

+
 import { ValidFirestoreKeys } from "types/Firestore";

 /**


Defined<T> is already defined in src/types/utility.ts so doesn't need to exist here

0xdevalias · 2021-01-14T05:26:04Z

RE: #801 (comment)

Even though I support this intent, it didn't seem to work right. The current implementation of this(null) works good because then you try to pass null there it doesn't like it. So a type string | null will not be accepted by TS. That is the main reasoning behind useVenueId change.

~~I'll open a sub-PR that fixes this properly shortly, just polishing it locally.~~

@mike-lvov See sub-PR #1076 for an implementation that fixes this properly, without needing to result to null hacks

…ery patterns + update related usages (#1076)

0xdevalias

Skimming over the changed files I only had 1 small request, then i'm happy for this PR to be merged.

Since I was the one who originally opened it, I can't actually approve it. But once you make the change mentioned here, feel free to approve and merge it.

(When squash-merging, make sure to keep the 'co-authored by' text it adds)

0xdevalias · 2021-01-14T09:54:38Z


  // Parent Events
-  const parentVenueEventsQuery: SparkleRFQConfig | undefined =
+  const parentVenueEventsQuery =


@mike-lvov can you add the explicit typing back here please

0xdevalias · 2021-01-14T09:55:40Z

+// @debt I think we could just use useParams() here instead of needing to loop through all of the venuePath, so long as the use :venueId in their path
+//   https://reactrouter.com/web/api/Hooks/useparams
+//     useParams returns an object of key/value pairs of URL parameters. Use it to access match.params of the current <Route>.


[OOS] see https://github.com/sparkletown/internal-sparkle-issues/issues/164

mike-lvov

LGTM!

qlty-cloud-legacy · 2021-01-14T10:42:03Z

Code Climate has analyzed commit ab6d832 and detected 6 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	6

View more on Code Climate.

Undo "Revert "Prevent pathological queries when doc is not specified (#…

7684ae3

…784)" (#797)" This reverts commit a5e6923.

0xdevalias added the ❌ do-not-merge PR should not be merged. Ask the person who added it for more details if the reason isn't clear. label Nov 25, 2020

0xdevalias assigned 0xdevalias and cadamsdotcom Nov 25, 2020

0xdevalias added the P0 - Emergency/Fire 🔥🔥🔥 Priority 0 - Emergency. Once reviewed/merged/tested, deploy straight to master/other prod envs label Nov 25, 2020

0xdevalias removed the P0 - Emergency/Fire 🔥🔥🔥 Priority 0 - Emergency. Once reviewed/merged/tested, deploy straight to master/other prod envs label Nov 25, 2020

cadamsdotcom unassigned 0xdevalias Nov 25, 2020

Merge branch 'staging' into devalias-fix-cadams-doc-or-collection-que…

e23524c

…ries

Merge branch 'staging' into devalias-fix-cadams-doc-or-collection-que…

715f2b7

…ries

0xdevalias added 🐛 bug A bug, error, issue, problem, etc within the platform. 💻 eng-QoL Engineering Quality of Life (QoL). Scripts, bots, automations, etc; that ensure our standards labels Dec 8, 2020

0xdevalias mentioned this pull request Dec 12, 2020

fix a bunch more performance related things (including a pathological query of all private chats) #988

Merged

Merge branch 'staging' into devalias-fix-cadams-doc-or-collection-que…

730db3c

…ries

cadamsdotcom marked this pull request as ready for review December 14, 2020 09:00

cadamsdotcom assigned denisdimitrov and unassigned cadamsdotcom Dec 14, 2020

cadamsdotcom and others added 2 commits December 17, 2020 16:25

Merge branch 'staging' into devalias-fix-cadams-doc-or-collection-que…

d21c4e7

…ries

Merge branch 'staging' into devalias-fix-cadams-doc-or-collection-que…

aebd3c0

…ries

mike-lvov reviewed Dec 22, 2020

View reviewed changes

Comment thread src/hooks/useFirestoreConnect.ts Outdated

Comment thread src/components/molecules/NavBar/NavBar.tsx Outdated

mike-lvov changed the title ~~Prevent pathological queries when doc is not specified - Fix~~ WIP: [FIX] Prevent pathological queries when doc is not specified Jan 4, 2021

mike-lvov changed the title ~~WIP: [FIX] Prevent pathological queries when doc is not specified~~ WIP: [IMPROVEMENT] Prevent pathological queries when doc is not specified Jan 4, 2021

0xdevalias mentioned this pull request Jan 5, 2021

[FIX] Chat display fix #1059

Merged

0xdevalias assigned mike-lvov and unassigned denisdimitrov Jan 11, 2021

Merge remote-tracking branch 'origin/staging' into devalias-fix-cadam…

1c65d4d

…s-doc-or-collection-queries

mike-lvov requested a deployment to feature-preview January 12, 2021 14:35 Waiting

qlty-cloud-legacy Bot reviewed Jan 12, 2021

View reviewed changes

Comment thread src/hooks/useFirestoreConnect.ts

mike-lvov requested a deployment to feature-preview January 13, 2021 13:32 Waiting

qlty-cloud-legacy Bot reviewed Jan 13, 2021

View reviewed changes

Comment thread src/hooks/useConnectCurrentEvent.ts

Comment thread src/hooks/useConnectUserPurchaseHistory.ts

Comment thread src/hooks/useConnectCurrentVenue.ts

This comment has been minimized.

Sign in to view

Removes pahological queries

97e96ce

mike-lvov force-pushed the devalias-fix-cadams-doc-or-collection-queries branch from 228abeb to 97e96ce Compare January 13, 2021 14:07

mike-lvov requested a deployment to feature-preview January 13, 2021 14:07 Waiting

qlty-cloud-legacy Bot reviewed Jan 13, 2021

View reviewed changes

Comment thread src/hooks/useConnectCurrentEvent.ts

Comment thread src/hooks/useConnectUserPurchaseHistory.ts

Comment thread src/hooks/useConnectCurrentVenue.ts

This comment has been minimized.

Sign in to view

0xdevalias mentioned this pull request Jan 14, 2021

[documentation] add code style priciples #1062

Closed

0xdevalias commented Jan 14, 2021

View reviewed changes

0xdevalias mentioned this pull request Jan 14, 2021

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

Merged

constrain useFirestoreConnect query types to enforce our firestore qu…

c9f7c8c

…ery patterns + update related usages (#1076)

mike-lvov requested a deployment to feature-preview January 14, 2021 09:26 Waiting

Adds const types

ddc3dcf

mike-lvov requested a deployment to feature-preview January 14, 2021 09:41 Waiting

0xdevalias commented Jan 14, 2021

View reviewed changes

Minor type fix

ab6d832

mike-lvov requested a deployment to feature-preview January 14, 2021 10:41 Waiting

mike-lvov approved these changes Jan 14, 2021

View reviewed changes

mike-lvov merged commit 24f697a into staging Jan 14, 2021

mike-lvov deleted the devalias-fix-cadams-doc-or-collection-queries branch January 14, 2021 10:46

		const siblingVenuesQuery: SparkleRFQConfig \| undefined = !!parentId
		const siblingVenuesQuery: SparkleRFQConfig = !!parentId

		const subvenuesQuery: SparkleRFQConfig \| undefined = !!venueId
		const subvenuesQuery: SparkleRFQConfig = !!venueId

	if (!venueId) return history.replace("/admin");
	if (!venueId) return

		const allValidQueries: SparkleRFQConfig[] = [
		const allValidQueries: SparkleRFQConfig = [

Conversation

0xdevalias commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xdevalias commented Nov 25, 2020

Uh oh!

cadamsdotcom commented Nov 25, 2020

Uh oh!

cadamsdotcom commented Dec 14, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mike-lvov commented Jan 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

mike-lvov commented Jan 13, 2021

Uh oh!

mike-lvov commented Jan 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xdevalias commented Jan 13, 2021

Uh oh!

mike-lvov commented Jan 13, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

0xdevalias commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xdevalias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xdevalias commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xdevalias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-lvov left a comment

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy Bot commented Jan 14, 2021

0xdevalias commented Nov 25, 2020 •

edited

Loading

0xdevalias commented Jan 14, 2021 •

edited

Loading

0xdevalias commented Jan 14, 2021 •

edited

Loading