fetch featured channels #1064

2018-03-05T20:18:59+01:00

daovist commented

2018-03-05 20:18:59 +01:00

(Migrated from github.com)

fetches data for featured channels and puts it into store.content.featuredUris

work in progress, channels are not displaying yet...

fetches data for featured channels and puts it into store.content.featuredUris work in progress, channels are not displaying yet...

daovist commented

2018-03-06 14:59:09 +01:00

(Migrated from github.com)

This is ready for a real review - @liamcardenas @seanyesmunt

I ended up separating channel data from the existing discover data entirely. The channels are below the normal discover data and they come in several seconds later so I added a second waiting message:

{hasContent && !hasChannels && <BusyMessage message={__('Fetching featured channels')} />}

Issue #1049 says "If the object key begins with an @ (indicating a channel) and the listed content array is empty" so I have an empty channel and a channel with one uri hard coded in the action to demonstrate an empty channel and one with addresses, which I will remove as this issue reaches completion.

Because I am splitting the list from the API into "content" and "channels", a channel with a non empty array is treated like content so it doesn't get the categoryLink like in the issue description -- I am turning the given channel key into ${channel}#${certificateId} when the channel data comes in.

This is ready for a real review - @liamcardenas @seanyesmunt I ended up separating channel data from the existing discover data entirely. The channels are below the normal discover data and they come in several seconds later so I added a second waiting message: `{hasContent && !hasChannels && <BusyMessage message={__('Fetching featured channels')} />}` Issue #1049 says "If the object key begins with an @ (indicating a channel) and the listed content array is empty" so I have an empty channel and a channel with one uri hard coded in the action to demonstrate an empty channel and one with addresses, which I will remove as this issue reaches completion. Because I am splitting the list from the API into "content" and "channels", a channel with a non empty array is treated like content so it doesn't get the categoryLink like in the issue description -- I am turning the given channel key into `${channel}#${certificateId}` when the channel data comes in.

kauffj (Migrated from github.com) requested changes 2018-03-06 22:11:27 +01:00

src/renderer/redux/actions/content.js

kauffj (Migrated from github.com) commented

2018-03-06 22:10:19 +01:00

I think if you use doFetchClaimsByChannel you can eliminate most or all of this code.

I think if you use `doFetchClaimsByChannel` you can eliminate most or all of this code.

src/renderer/redux/reducers/content.js

kauffj (Migrated from github.com) commented

2018-03-06 22:11:12 +01:00

This looks like a new copy of claims-in-channel data, which would be a DRY break. Use the same place that other channel claim data is stored. Please look at ACTIONS.FETCH_CHANNEL_CLAIMS_COMPLETED.

This looks like a new copy of claims-in-channel data, which would be a DRY break. Use the same place that other channel claim data is stored. Please look at `ACTIONS.FETCH_CHANNEL_CLAIMS_COMPLETED`.

daovist (Migrated from github.com) reviewed 2018-03-07 01:56:01 +01:00

src/renderer/redux/reducers/content.js

daovist (Migrated from github.com) commented

2018-03-07 01:56:01 +01:00

That's what I was going for all week but couldn't get all the parts together. I could never get the component to re-render when the channels came in, or couldn't load from channels correctly while not interfering with the subscriptions page or having to reset before viewing either page to not mix featured and subscribed channels.

I pushed this because it worked. I will try more of that tomorrow and push.

That's what I was going for all week but couldn't get all the parts together. I could never get the component to re-render when the channels came in, or couldn't load from channels correctly while not interfering with the subscriptions page or having to reset before viewing either page to not mix featured and subscribed channels. I pushed this because it worked. I will try more of that tomorrow and push.

daovist (Migrated from github.com) reviewed 2018-03-07 17:22:28 +01:00

src/renderer/redux/reducers/content.js

daovist (Migrated from github.com) commented

2018-03-07 17:22:28 +01:00

@kauffj a more thorough explanation:

ACTIONS.FETCH_CHANNEL_CLAIMS_COMPLETED stores claim data in state.claims.claimsByChannel

rewriting the reducer to store the data in state.content.featuredUris when requested by the Discover page rather than the Subscriptions page seems like it will add as much code and complexity as my approach of adding 3 state.content properties, 1 action, and 2 reducers

if the Discover page is putting featured channels in state.claims.claimsByChannel then both featured and subscribed channels will be displayed on the Subscriptions page

and on the Discover page if/when the users goes back

each page could filter out the other's claim data in the view but that seems hackish

not to mention this would also require adding all the subscription fetching code to the discover component, which is a huge mess with the following comment:

// setHasFetchedSubscriptions is a terrible hack // it allows the subscriptions to load correctly when refresing on the subscriptions page // currently the page is rendered before the state is rehyrdated // that causes this component to be rendered with zero savedSubscriptions // we need to wait until persist/REHYDRATE has fired before rendering the page

https://github.com/lbryio/lbry-app/blob/master/src/renderer/page/subscriptions/view.jsx

this is the road I went down--and ran into rendering issues I still haven't figured out--before becoming convinced that treating Featured Channels as its own feature worthy of its own place in the data model was simpler than trying to implement it using functions built for Featured Uris and Subscriptions

I may not need fetchingFeaturedChannels and am not yet using fetchingFeaturedChannelsFailed so this doesn't need to be a full copy of featuredUris's data structure, I was just trying to get something that made sense on here for y'all to look at

I am happy to refactor the Subscriptions page but that seems like a bigger issue than Featured Channels

or I could still be missing a simple solution, this part of the app is still new to me

@kauffj a more thorough explanation: `ACTIONS.FETCH_CHANNEL_CLAIMS_COMPLETED` stores claim data in `state.claims.claimsByChannel` rewriting the reducer to store the data in `state.content.featuredUris` when requested by the Discover page rather than the Subscriptions page seems like it will add as much code and complexity as my approach of adding 3 `state.content` properties, 1 action, and 2 reducers if the Discover page is putting featured channels in `state.claims.claimsByChannel` then both featured and subscribed channels will be displayed on the Subscriptions page and on the Discover page if/when the users goes back each page could filter out the other's claim data in the view but that seems hackish not to mention this would also require adding all the subscription fetching code to the discover component, which is a huge mess with the following comment: `// setHasFetchedSubscriptions is a terrible hack // it allows the subscriptions to load correctly when refresing on the subscriptions page // currently the page is rendered before the state is rehyrdated // that causes this component to be rendered with zero savedSubscriptions // we need to wait until persist/REHYDRATE has fired before rendering the page` https://github.com/lbryio/lbry-app/blob/master/src/renderer/page/subscriptions/view.jsx this is the road I went down--and ran into rendering issues I still haven't figured out--before becoming convinced that treating Featured Channels as its own feature worthy of its own place in the data model was simpler than trying to implement it using functions built for Featured Uris and Subscriptions I may not need `fetchingFeaturedChannels` and am not yet using `fetchingFeaturedChannelsFailed` so this doesn't need to be a full copy of `featuredUris`'s data structure, I was just trying to get something that made sense on here for y'all to look at I am happy to refactor the Subscriptions page but that seems like a bigger issue than Featured Channels or I could still be missing a simple solution, this part of the app is still new to me

kauffj (Migrated from github.com) reviewed 2018-03-08 00:03:51 +01:00

src/renderer/redux/reducers/content.js

kauffj (Migrated from github.com) commented

2018-03-08 00:03:51 +01:00

@daovist

Not storing the same data in two different places or two different ways is about as fundamental as it gets when it comes to DRY. It is exceedingly unlikely we'll accept a solution that involves storing the same data in two places or in two different ways (caching for performance reasons is the only exception I can think of).
If a simpler data model comes at the cost of more code that is entirely acceptable.
I suspect there is a misunderstanding (or someone else is doing something very wrong), if data additions to claimsByChannel are affecting subscriptions. claimsByChannel is where all data is stored related to claims in a channel. Accessing any channel page stores data there.
I'm not following a lot of your comments. I don't understand how this is related to subscriptions at all. I don't understand what you mean with your comments related to filtering. I strongly suspect this functionality should be doable with 0 additional actions and 0 additional Redux state values.

@daovist 1) Not storing the same data in two different places or two different ways is about as fundamental as it gets when it comes to DRY. It is exceedingly unlikely we'll accept a solution that involves storing the same data in two places or in two different ways (caching for performance reasons is the only exception I can think of). 2) If a simpler data model comes at the cost of more code that is entirely acceptable. 3) I suspect there is a misunderstanding (or someone else is doing something very wrong), if data additions to `claimsByChannel` are affecting subscriptions. `claimsByChannel` is where all data is stored related to claims in a channel. Accessing any channel page stores data there. 4) I'm not following a lot of your comments. I don't understand how this is related to subscriptions at all. I don't understand what you mean with your comments related to filtering. I strongly suspect this functionality should be doable with 0 additional actions and 0 additional Redux state values.

daovist commented

2018-03-08 17:56:40 +01:00

(Migrated from github.com)

@kauffj here is a new version based on what you said. It only alters the one action doFetchFeaturedUris to call doFetchClaimsByChannel and uses existing selectors. All of the new logic is in the render function of DiscoverPage.

There are two hardcoded channels added to Uris in doFetchFeaturedUris to demonstrate channels with empty and non-empty arrays, which I will remove if this code is acceptable.

Edit: actually, a bunch of the useless code is still in there, but not being used, which I'm removing now...

@kauffj here is a new version based on what you said. It only alters the one action `doFetchFeaturedUris` to call `doFetchClaimsByChannel` and uses existing selectors. All of the new logic is in the `render` function of `DiscoverPage`. There are two hardcoded channels added to `Uris` in `doFetchFeaturedUris` to demonstrate channels with empty and non-empty arrays, which I will remove if this code is acceptable. Edit: actually, a bunch of the useless code is still in there, but not being used, which I'm removing now...

kauffj (Migrated from github.com) requested changes 2018-03-08 23:55:11 +01:00

kauffj (Migrated from github.com) left a comment

@daovist great refactor! One more potential improvement.

src/renderer/page/discover/view.jsx

					
				@ -223,2 +223,4 @@

				class DiscoverPage extends React.PureComponent {

				  constructor(props) {

kauffj (Migrated from github.com) commented

2018-03-08 23:54:42 +01:00

Can this logic be either moved into <FeaturedCategory> and/or moved into a selector that is called by <FeaturedCategory>?

Can this logic be either moved into `<FeaturedCategory>` and/or moved into a selector that is called by `<FeaturedCategory>`?

daovist (Migrated from github.com) reviewed 2018-03-09 15:01:30 +01:00

src/renderer/page/discover/view.jsx

					
				@ -223,2 +223,4 @@

				class DiscoverPage extends React.PureComponent {

				  constructor(props) {

daovist (Migrated from github.com) commented

2018-03-09 15:01:30 +01:00

I moved all this logic into selectDiscover and am calling it from the Discover page and it's working correctly. Calling it from <FeaturedCategory> doesn't make sense to me because the selector returns an object which is mapped to many <FeaturedCategory>s in the view.

I can kind of see doing to logic for each category separately but I would need to think about it and it doesn't seem like it will be nearly as clean as this.

I moved all this logic into `selectDiscover` and am calling it from the `Discover` page and it's working correctly. Calling it from `<FeaturedCategory>` doesn't make sense to me because the selector returns an object which is mapped to many `<FeaturedCategory>`s in the view. I can kind of see doing to logic for each category separately but I would need to think about it and it doesn't seem like it will be nearly as clean as this.

tzarebczan commented

2018-03-14 16:08:25 +01:00

(Migrated from github.com)

@liamcardenas / @daovist what's the status on this PR?

daovist commented

2018-03-15 16:11:44 +01:00

(Migrated from github.com)

@tzarebczan I think it's good to go given that @kauffj is cool with <DiscoverPage> the selector rather than the <FeaturedCategory>

and nobody else has any objections

one nuance of this code is it assumes it's getting just the @channel and not @channel#claim_id -- if someone put @channel#claim_id in the original json it would mess things up, but I could fix this easy enough

I still have two channels hardcoded into the action so others can see the functionality of this branch in dev before we start including channels in the lbry.io API -- happy to take them out when this is merge-ready

@tzarebczan I think it's good to go given that @kauffj is cool with `<DiscoverPage>` the selector rather than the `<FeaturedCategory>` and nobody else has any objections one nuance of this code is it assumes it's getting just the `@channel` and not `@channel#claim_id` -- if someone put `@channel#claim_id` in the original json it would mess things up, but I could fix this easy enough I still have two channels hardcoded into the action so others can see the functionality of this branch in dev before we start including channels in the lbry.io API -- happy to take them out when this is merge-ready

kauffj commented

2018-03-15 16:26:14 +01:00

(Migrated from github.com)

@daovist I think an approach where <FeaturedCategory> initiates the channel fetches is mildly better, but I won't block over this. (It's better because basically deferring execution until you're in the lowest-level component that needs it results in tighter and more logical coupling. It's also the "React way".)

I'm not sure sure why a new selector is necessary either. If a new one is necessary, then presumably selectFeaturedUris can be dropped?

@daovist I think an approach where `<FeaturedCategory>` initiates the channel fetches is mildly better, but I won't block over this. (It's better because basically deferring execution until you're in the lowest-level component that needs it results in tighter and more logical coupling. It's also the "React way".) I'm not sure sure why a new selector is necessary either. If a new one is necessary, then presumably `selectFeaturedUris` can be dropped?

daovist commented

2018-03-15 17:47:02 +01:00

(Migrated from github.com)

selectDiscover uses selectFeaturedUris

I understand what you @kauffj are/were saying much better now and agree. It means leaving the Discover page mostly as it was and grabbing channel data in another selector that uses similar logic to what I've got here

my concern is that changing <FeaturedCategory> for channels would affect <SubscriptionsPage>

(also I realize I need to refactor FC into its own top level component either way)

a small thing is the lbry.io API would need the full @channel#claim_id which shouldn't be much of an issue since people can copypasta

`selectDiscover` uses `selectFeaturedUris` I understand what you @kauffj are/were saying much better now and agree. It means leaving the Discover page mostly as it was and grabbing channel data in another selector that uses similar logic to what I've got here my concern is that changing `<FeaturedCategory>` for channels would affect `<SubscriptionsPage>` (also I realize I need to refactor FC into its own top level component either way) a small thing is the lbry.io API would need the full `@channel#claim_id` which shouldn't be much of an issue since people can copypasta

liamcardenas commented

2018-03-15 19:37:55 +01:00

(Migrated from github.com)

How are we going to ensure that we don't break the homepage for old versions of the app by returning channel names via the api?

kauffj commented

2018-03-15 19:43:36 +01:00

(Migrated from github.com)

@liamcardenas breaking old app versions is acceptable - upgrading is basically mandatory during the beta.

👍 1

liamcardenas (Migrated from github.com) requested changes 2018-03-15 21:53:58 +01:00

liamcardenas (Migrated from github.com) left a comment

Can you please:

(1) merge the latest version of master into your branch
(2) do the command yarn run format so that the code is properly formatted

Thanks!

Can you please: (1) merge the latest version of master into your branch (2) do the command `yarn run format` so that the code is properly formatted Thanks!

src/renderer/page/discover/index.js

					
				@ -2,11 +2,18 @@ import React from 'react';

				import { connect } from 'react-redux';

				import { doFetchFeaturedUris } from 'redux/actions/content';

				import { selectFeaturedUris, selectFetchingFeaturedUris } from 'redux/selectors/content';

liamcardenas (Migrated from github.com) commented

2018-03-15 21:38:09 +01:00

you don't use this

daovist commented

2018-03-19 21:14:27 +01:00

(Migrated from github.com)

I got rid of the imports and select properties from /page/discover/index.js as @liamcardenas requested and that broke the whole thing. I tried putting it back the way it was before I moved the logic in the view render function to its own selector and it worked again. So I worked piece by piece back to how it was when it was working and found that including extra selectors in the index file's select function is the difference between featured channels showing up when they arrive and them not showing (until you click subscriptions and back to discover; if the channel data is already in the state, it's fine, the problem is not triggering cWRP or render when selectDiscover updates)

working:

fetchingFeaturedUris: selectFetchingFeaturedUris(state), categories: selectDiscover(state), featuredUris: selectFeaturedUris(state), claimsById: selectClaimsById(state), claimsByChannel: selectAllClaimsByChannel(state),

not working:

fetchingFeaturedUris: selectFetchingFeaturedUris(state), categories: selectDiscover(state), // featuredUris: selectFeaturedUris(state), // claimsById: selectClaimsById(state), // claimsByChannel: selectAllClaimsByChannel(state),

the extra selectors aren't even declared in the view's render function:

const { fetchingFeaturedUris, categories } = this.props;

Every time I think I understand React, I find something new and confusing. I'm gonna read react-indepth this evening/tonight and see if this doesn't make more sense tomorrow

I'm also gonna retry the @kauffj approach of calling doFetchClaimsByChannel from <FeaturedChannel> and dealing with an empty names prop

I got rid of the imports and select properties from `/page/discover/index.js` as @liamcardenas requested and that broke the whole thing. I tried putting it back the way it was before I moved the logic in the view render function to its own selector and it worked again. So I worked piece by piece back to how it was when it was working and found that including extra selectors in the index file's select function is the difference between featured channels showing up when they arrive and them not showing (until you click subscriptions and back to discover; if the channel data is already in the state, it's fine, the problem is not triggering cWRP or render when `selectDiscover` updates) working: ` fetchingFeaturedUris: selectFetchingFeaturedUris(state), categories: selectDiscover(state), featuredUris: selectFeaturedUris(state), claimsById: selectClaimsById(state), claimsByChannel: selectAllClaimsByChannel(state), ` not working: ` fetchingFeaturedUris: selectFetchingFeaturedUris(state), categories: selectDiscover(state), // featuredUris: selectFeaturedUris(state), // claimsById: selectClaimsById(state), // claimsByChannel: selectAllClaimsByChannel(state), ` the extra selectors aren't even declared in the view's render function: ` const { fetchingFeaturedUris, categories } = this.props; ` Every time I think I understand React, I find something new and confusing. I'm gonna read react-indepth this evening/tonight and see if this doesn't make more sense tomorrow I'm also gonna retry the @kauffj approach of calling `doFetchClaimsByChannel` from `<FeaturedChannel>` and dealing with an empty `names` prop

liamcardenas (Migrated from github.com) requested changes 2018-03-27 19:51:19 +02:00

liamcardenas (Migrated from github.com) left a comment

I tested this and it works beautifully! We can consider re-architecting this to request the change in the component, but I think that is a marginal improvement at best.

I made some code suggestions that should keep you busy for a while. Once we get all of those resolved, I will be happy to merge this.

(also please resolve merge conflicts)

Thanks!

I tested this and it works beautifully! We can consider re-architecting this to request the change in the component, but I think that is a marginal improvement at best. I made some code suggestions that should keep you busy for a while. Once we get all of those resolved, I will be happy to merge this. (also please resolve merge conflicts) Thanks!

src/renderer/page/discover/index.js

					
				@ -10,0 +11,4 @@

				  categories: selectDiscover(state),

				  // we shouldn't need these

				  featuredUris: selectFeaturedUris(state),

liamcardenas (Migrated from github.com) commented

2018-03-27 19:47:25 +02:00

if we don't need them, can you take them out?

src/renderer/page/discover/view.jsx

liamcardenas (Migrated from github.com) commented

2018-03-27 19:48:35 +02:00

do you need to split it because you are extracting the channel name from a uri? look into using parseURI

do you need to split it because you are extracting the channel name from a uri? look into using `parseURI`

src/renderer/redux/actions/content.js

					
				@ -78,21 +78,25 @@ export function doFetchFeaturedUris() {

				    });

				    const success = ({ Uris }) => {

				      Uris['@LupoTV'] = [];

liamcardenas (Migrated from github.com) commented

2018-03-27 19:23:58 +02:00

I appreciate you leaving this in for testing purposes, obviously remove this when it is ready to merge

src/renderer/redux/actions/content.js

					
				@ -80,2 +80,4 @@

				    const success = ({ Uris }) => {

				      Uris['@LupoTV'] = [];

				      let urisToResolve = [];

				      let actions = [];

liamcardenas (Migrated from github.com) commented

2018-03-27 19:16:17 +02:00

use const, when possible

src/renderer/redux/selectors/discover.js

					
				@ -0,0 +6,4 @@

				  [selectFeaturedUris, selectClaimsById, selectAllClaimsByChannel],

				  (featuredUris, claimsById, claimsByChannel) => {

				    let categories = featuredUris;

				    if (!!categories && !!claimsByChannel) {

liamcardenas (Migrated from github.com) commented

2018-03-27 19:27:18 +02:00

unnecessary to do !!

unnecessary to do `!!`

src/renderer/redux/selectors/discover.js

					
				@ -0,0 +14,4 @@

				        }

				      });

				      Object.keys(claimsByChannel).forEach(key => {

				        if (channels.includes(key)) {

liamcardenas (Migrated from github.com) commented

2018-03-27 19:35:24 +02:00

you could take out the other forEach loop if you change this line to
if(categories[key].indexOf('@') === 0)

you could take out the other forEach loop if you change this line to `if(categories[key].indexOf('@') === 0)`

src/renderer/redux/selectors/discover.js

					
				@ -0,0 +18,4 @@

				          delete categories[key];

				          const ids = claimsByChannel[key][1];

				          let uris = [];

				          const newKey = `${key}#${claimsById[ids[0]].value.publisherSignature.certificateId}`;

liamcardenas (Migrated from github.com) commented

2018-03-27 19:37:27 +02:00

instead of using string interpolation, the proper way to do with is with the buildURI function

(I know I told you this method was fine earlier, sorry about that)

instead of using string interpolation, the proper way to do with is with the `buildURI` function (I know I told you this method was fine earlier, sorry about that)

src/renderer/redux/selectors/discover.js

					
				@ -0,0 +19,4 @@

				          const ids = claimsByChannel[key][1];

				          let uris = [];

				          const newKey = `${key}#${claimsById[ids[0]].value.publisherSignature.certificateId}`;

				          ids.forEach(id => {

liamcardenas (Migrated from github.com) commented

2018-03-27 19:44:41 +02:00

instead of all of these loops, you should learn how to use map, reduce, filter, etc.

i dont think this would block merging, however, here is a rough sketch of how i would do this

uri = ids.filter(id => claimsById[id]).map(id => `${claimsById[id.name}#${claimsById[id.claim_id}`);

and in fact you could even set

categories[newKey] = ids.filter(id => claimsById[id]).map(id => `${claimsById[id.name}#${claimsById[id.claim_id}`);

this could cut out a lot of this code and make it shorter. It's something you should play around with.

instead of all of these loops, you should learn how to use `map`, `reduce`, `filter`, etc. i dont think this would block merging, however, here is a rough sketch of how i would do this ``` uri = ids.filter(id => claimsById[id]).map(id => `${claimsById[id.name}#${claimsById[id.claim_id}`); ``` and in fact you could even set ``` categories[newKey] = ids.filter(id => claimsById[id]).map(id => `${claimsById[id.name}#${claimsById[id.claim_id}`); ``` this could cut out a lot of this code and make it shorter. It's something you should play around with.

src/renderer/redux/selectors/discover.js

					
				@ -0,0 +22,4 @@

				          ids.forEach(id => {

				            const claim = claimsById[id] ? claimsById[id] : null;

				            if (claim) {

				              uris.push(`${claim.name}#${claim.claim_id}`);

liamcardenas (Migrated from github.com) commented

2018-03-27 19:37:43 +02:00

same here, use buildURI

src/renderer/redux/selectors/discover.js

					
				@ -0,0 +29,4 @@

				        }

				      });

				    }

				    console.log("select:", categories);

liamcardenas (Migrated from github.com) commented

2018-03-27 19:44:56 +02:00

obviously remove when ready to merge :)

liamcardenas commented

2018-03-27 19:55:08 +02:00

(Migrated from github.com)

dang, i accidentally was looking at this one instead of the other one. In the future, it would be best to close PRs that are no longer relevant. I will close this if I feel the other one is better.

liamcardenas commented

2018-03-27 20:12:14 +02:00

(Migrated from github.com)

yep, the other one was better.