fix: fetch subscriptions if there are no claims loaded #1652

Merged

neb-b merged 1 commit from subscriptions-fix into master

2018-06-20 21:11:41 +02:00

neb-b commented

2018-06-20 08:52:10 +02:00

(Migrated from github.com)

#1516

Sometimes, a claim id would be added to byId in redux state even if there are no claims loaded. Maybe that shouldn't happen? This ensures that the claim has no uris loaded before fetching, before we were just checking if the claim id existed in redux or not.

#1516 Sometimes, a claim id would be added to `byId` in redux state even if there are no claims loaded. Maybe that shouldn't happen? This ensures that the claim has no uris loaded before fetching, before we were just checking if the claim id existed in redux or not.

kauffj (Migrated from github.com) reviewed 2018-06-20 08:52:10 +02:00

tzarebczan commented

2018-06-20 15:29:59 +02:00

(Migrated from github.com)

Somehow this magic seems to work.

kauffj commented

2018-06-20 15:54:07 +02:00

(Migrated from github.com)

@seanyesmunt welcome to complex application state. To quote the famous programer T.S. Elliot:

This is the way the simplicity ends.
Not with a bang but a whimper.

When you've reached sufficient complexity, you'll inevitably reach a point where you understand that a function receives bad input but you don't understand why or how. Sometimes, as in this case, it's possible to adjust or tweak the function to accept or handle the unexpected data.

Now you have a choice.

You can make this fix, which is quick and easy. But you haven't addressed the underlying cause. Something is causing a data state to exist that doesn't make sense. This hasn't solved that problem, it's just hidden it. But that's not the only problem. Now even if the underlying cause is fixed, there's no connection between that fix and this code. The most likely scenario is that this check remains for as long as this file exists, because no one will ever come across that line and think to remove it. Which means the complexity and cost of carrying that line (which admittedly is fairly low in this specific case), persists for a very long time.

This might sound like I'm arguing against this change, and while I kind of am, the other choice also sucks. Because the other choice is track down why and how you've got a state that doesn't make sense, which might be idiosyncratic and a big time-waster, when you could just make this simple one-line fix and get back to doing something people actually care about.

There is a third-way, which is to add these types of checks (which let you get back to doing useful stuff), but code them in such a way that makes it more feasible to eventually solve the underlying problem as well as not let this check get stuck once the problem is fixed.

Here's an example of what that might look like:

/*
This check added 6/20/18 to fix function receiving empty claims unexpectedly.
Once the check statement below hasn't been seen for time period X, the below check should be removed.
*/
if (claim.claims.length) {
  subscriptionClaimMap[claim.uri] = 1;
} else if (app.env === ENV.DEVELOPLMENT) {
  console.log("note about unexpected thing happening and/or throw an exception and/or show a relevant trace"); 
}

@seanyesmunt welcome to complex application state. To quote the famous programer T.S. Elliot: > This is the way the simplicity ends. > Not with a bang but a whimper. When you've reached sufficient complexity, you'll inevitably reach a point where you understand that a function receives bad input but you don't understand why or how. Sometimes, as in this case, it's possible to adjust or tweak the function to accept or handle the unexpected data. Now you have a choice. You can make this fix, which is quick and easy. But you haven't addressed the underlying cause. _Something_ is causing a data state to exist that doesn't make sense. This hasn't solved that problem, it's just hidden it. But that's not the only problem. Now even if the underlying cause _is_ fixed, there's no connection between that fix and this code. The most likely scenario is that this check remains for as long as this file exists, because no one will ever come across that line and think to remove it. Which means the complexity and cost of carrying that line (which admittedly is fairly low in this specific case), persists for a very long time. This might sound like I'm arguing against this change, and while I kind of am, the other choice _also_ sucks. Because the other choice is track down why and how you've got a state that doesn't make sense, which might be idiosyncratic and a big time-waster, when you could just make this simple one-line fix and get back to doing something people actually care about. There is a third-way, which is to add these types of checks (which let you get back to doing useful stuff), but code them in such a way that makes it more feasible to eventually solve the underlying problem as well as not let this check get stuck once the problem is fixed. Here's an example of what that might look like: ``` /* This check added 6/20/18 to fix function receiving empty claims unexpectedly. Once the check statement below hasn't been seen for time period X, the below check should be removed. */ if (claim.claims.length) { subscriptionClaimMap[claim.uri] = 1; } else if (app.env === ENV.DEVELOPLMENT) { console.log("note about unexpected thing happening and/or throw an exception and/or show a relevant trace"); } ```

👍 1

kauffj commented

2018-06-20 15:55:57 +02:00

(Migrated from github.com)

Lol, I may have went overboard on my comment if this is just due to empty channels. But hopefully the lesson made sense even if it didn't apply here.

tzarebczan commented

2018-06-20 15:58:40 +02:00

(Migrated from github.com)

Nope, it's more than just empty channels. I'm going to help figure this beast out one day :)

neb-b commented

2018-06-20 21:11:33 +02:00

(Migrated from github.com)

I'm going to merge this now, but continue to work on it today/tonight.

Figured it is better to add now because it is currently a really bad experience

I'm going to merge this now, but continue to work on it today/tonight. Figured it is better to add now because it is currently a really bad experience

neb-b commented

2018-08-22 06:14:09 +02:00

(Migrated from github.com)

A better solution was to just get rid of all of this in the component.
https://github.com/lbryio/lbry-desktop/pull/1897/files

A better solution was to just get rid of all of this in the component. https://github.com/lbryio/lbry-desktop/pull/1897/files