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

Internal subscriptions #1424

Merged
merged 1 commit into from May 15, 2018

Conversation

Projects
None yet
6 participants
@seanyesmunt
Member

seanyesmunt commented May 2, 2018

Adds subscriptions to our internal-api database

Notes

Ideally this should live in lbry-redux. It seems like there is still some work moving lbry.call over, mostly with auth stuff. And we need to figure out how we want to interact between app code and lbry-redux code. Currently the the subscriptions actions we are using doPurchaseUri which doesn't exist in lbry-redux. It shouldn't be too difficult to move over, but will just take some planning. I thought it was out of the scope for this PR.

Implementation

There are 3 main scenarios that cause some extra logic re: respecting users privacy. If a user decides not to share app usage, we will not make any calls to the db. If they are sharing data with us, this is what happens:

0 subs in db, but x subs in redux:
- This will happen on the initial run, or if users decide to start sharing data after they have subscribed to a channel
- In this case we will populate every sub in redux into the db


x subs in db, but 0 subs in redux:
- This will happen if a user clears there cache
- Populate all subs in db into redux
- This will cause issues with users who have subscribed, then stopped sharing data, and unsubscribed from everyone, then started sharing data again. They will be re-subscribed (I think this is ok for now)

x subs in db, y subs in redux:
- There is some mismatch from failing api calls or stopping/starting to share app usage
- Default to whats in redux
- Subscribe to any missing channels in the db
- Unsubscribe to any channels in the db that aren't in redux
@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot May 2, 2018

Codacy Here is an overview of what got changed by this pull request:

Clones added
============
- src/renderer/redux/actions/subscriptions.js  2
         

See the complete overview on Codacy

codacy-bot commented May 2, 2018

Codacy Here is an overview of what got changed by this pull request:

Clones added
============
- src/renderer/redux/actions/subscriptions.js  2
         

See the complete overview on Codacy

@lbryio lbryio deleted a comment from codacy-bot May 2, 2018

@seanyesmunt seanyesmunt requested a review from kauffj May 2, 2018

@lbry-bot lbry-bot assigned kauffj and unassigned seanyesmunt May 2, 2018

@seanyesmunt seanyesmunt requested a review from IGassmann May 2, 2018

@seanyesmunt seanyesmunt requested a review from liamcardenas May 2, 2018

@seanyesmunt seanyesmunt removed the request for review from IGassmann May 2, 2018

@seanyesmunt seanyesmunt changed the title from [WIP] Internal subscriptions to Internal subscriptions May 3, 2018

};
class Page extends React.PureComponent<Props, State> {
static getDerivedStateFromProps(nextProps: Props, prevState: State) {

This comment has been minimized.

@seanyesmunt

seanyesmunt May 5, 2018

Member

React recommends getDerivedStateFromProps moving forward instead of componentWillReceiveProps

@seanyesmunt

seanyesmunt May 5, 2018

Member

React recommends getDerivedStateFromProps moving forward instead of componentWillReceiveProps

@@ -34,6 +34,7 @@
"singleQuote": true
}],
"func-names": ["warn", "as-needed"],
"jsx-a11y/label-has-for": 0
"jsx-a11y/label-has-for": 0,
"import/prefer-default-export": 0

This comment has been minimized.

@seanyesmunt

seanyesmunt May 7, 2018

Member

IMO this isn't needed. Mostly for util files, I don't want to add the first one as a default export if I think there may be more functions added later.

@seanyesmunt

seanyesmunt May 7, 2018

Member

IMO this isn't needed. Mostly for util files, I don't want to add the first one as a default export if I think there may be more functions added later.

};
export const doChannelUnsubscribe = (subscription: Subscription) => (dispatch: Dispatch) => {
export const setSubscriptionLatest = (subscription: Subscription, uri: string) => (

This comment has been minimized.

@seanyesmunt

seanyesmunt May 7, 2018

Member

Didn't touch this, lint was telling me to move it

@seanyesmunt

seanyesmunt May 7, 2018

Member

Didn't touch this, lint was telling me to move it

@@ -114,40 +228,80 @@ export const doCheckSubscription = (subscription: Subscription, notify?: boolean
});
};
export const setSubscriptionLatest = (subscription: Subscription, uri: string) => (
export const setSubscriptionNotifications = (notifications: SubscriptionNotifications) => (

This comment has been minimized.

@seanyesmunt

seanyesmunt May 7, 2018

Member

Same here, just re-ordering

@seanyesmunt

seanyesmunt May 7, 2018

Member

Same here, just re-ordering

}
};
export const doCheckSubscriptions = () => (

This comment has been minimized.

@seanyesmunt

seanyesmunt May 7, 2018

Member

also this, just moved it

@seanyesmunt

seanyesmunt May 7, 2018

Member

also this, just moved it

@seanyesmunt seanyesmunt removed the request for review from liamcardenas May 7, 2018

@lyoshenka lyoshenka added this to the May 14 (app) milestone May 7, 2018

@kauffj

Generally looks good. Biggest questions by far are around correctness of subscription syncing logic.

@@ -145,6 +144,10 @@ class FileList extends React.PureComponent<Props, State> {
const { sortBy } = this.state;
const content = [];
if (!fileInfos) {
return null;

This comment has been minimized.

@kauffj

kauffj May 7, 2018

Member

Should this be null or an empty message?

(Genuinely not sure)

@kauffj

kauffj May 7, 2018

Member

Should this be null or an empty message?

(Genuinely not sure)

This comment has been minimized.

@seanyesmunt

seanyesmunt May 8, 2018

Member

I don't think it should be an empty message. If this is null, the data is (most likely) being fetched. An empty message would just flash quickly then go away when we start to render.

@seanyesmunt

seanyesmunt May 8, 2018

Member

I don't think it should be an empty message. If this is null, the data is (most likely) being fetched. An empty message would just flash quickly then go away when we start to render.

This comment has been minimized.

@kauffj

kauffj May 8, 2018

Member

👍

@kauffj

kauffj May 8, 2018

Member

👍

Show outdated Hide outdated src/renderer/constants/action_types.js
Show outdated Hide outdated src/renderer/page/channel/view.jsx
!!savedSubscriptions.length &&
(subscriptions.length !== savedSubscriptions.length || someClaimsNotLoaded);
let claimList = [];
subscriptionClaims.forEach(claimData => {

This comment has been minimized.

@kauffj

kauffj May 7, 2018

Member

Should this be in render() or a selector?

@kauffj

kauffj May 7, 2018

Member

Should this be in render() or a selector?

This comment has been minimized.

@seanyesmunt

seanyesmunt May 8, 2018

Member

Good point, it should just be in a selector

@seanyesmunt

seanyesmunt May 8, 2018

Member

Good point, it should just be in a selector

This comment has been minimized.

@seanyesmunt

seanyesmunt May 8, 2018

Member

Actually upon closer look I think it's ok if it's inside render(). I kept it inside this in this format so we can check if a subscription uri has any fetched claims in the componentDidUpdate

Since we are already passing them in this format, I don't think it makes sense to create a separate selctor to pass in the same data in a different format (I think)

@seanyesmunt

seanyesmunt May 8, 2018

Member

Actually upon closer look I think it's ok if it's inside render(). I kept it inside this in this format so we can check if a subscription uri has any fetched claims in the componentDidUpdate

Since we are already passing them in this format, I don't think it makes sense to create a separate selctor to pass in the same data in a different format (I think)

This comment has been minimized.

@kauffj

kauffj May 8, 2018

Member

👍

@kauffj

kauffj May 8, 2018

Member

👍

Show outdated Hide outdated src/renderer/redux/actions/subscriptions.js
Show outdated Hide outdated src/renderer/redux/actions/subscriptions.js
Show outdated Hide outdated src/renderer/redux/selectors/subscriptions.js
Show outdated Hide outdated src/renderer/util/dom.js
Show outdated Hide outdated src/renderer/util/dom.js

@lbry-bot lbry-bot assigned seanyesmunt and unassigned seanyesmunt May 7, 2018

@seanyesmunt

This comment has been minimized.

Show comment
Hide comment
@seanyesmunt

seanyesmunt May 10, 2018

Member

@kauffj ready for more feedback

Member

seanyesmunt commented May 10, 2018

@kauffj ready for more feedback

@kauffj

Looks very close.

Show outdated Hide outdated src/renderer/constants/action_types.js
Show outdated Hide outdated src/renderer/constants/settings.js
Show outdated Hide outdated src/renderer/redux/actions/subscriptions.js
// Actual claim type has more values than this
// Add them as they are used
export type Claim = {

This comment has been minimized.

@kauffj

kauffj May 10, 2018

Member

Should Subscription also be a standalone type file like Claim?

@kauffj

kauffj May 10, 2018

Member

Should Subscription also be a standalone type file like Claim?

This comment has been minimized.

@seanyesmunt
@seanyesmunt
storedSubscriptions.forEach(sub => {
if (!reduxSubMap[sub.claim_id]) {
const uri = `lbry://${sub.channel_name}#${sub.claim_id}`;
subscriptionsToReturn.push({ uri, channelName: sub.channel_name });

This comment has been minimized.

@kauffj

kauffj May 10, 2018

Member

I'm mildly surprised this works. If subscriptionsToReturn is an array of Subscription, doesn't this need to include the latest property?

@kauffj

kauffj May 10, 2018

Member

I'm mildly surprised this works. If subscriptionsToReturn is an array of Subscription, doesn't this need to include the latest property?

This comment has been minimized.

@seanyesmunt

seanyesmunt May 10, 2018

Member

latest isn't required. It will populate it in doCheckSubscriptions. We are just adding them to redux, which is where it looks when it's checking for the latest.

@seanyesmunt

seanyesmunt May 10, 2018

Member

latest isn't required. It will populate it in doCheckSubscriptions. We are just adding them to redux, which is where it looks when it's checking for the latest.

This comment has been minimized.

@kauffj

kauffj May 15, 2018

Member

I'm fairly sure this is my misunderstanding, but I still don't understand why Flow type checking wouldn't be forcing storedSubscriptions to be Array<Subscription> and thus require the latest property.

(Going to merge anyway, but if you know the answer would appreciate the education :) )

@kauffj

kauffj May 15, 2018

Member

I'm fairly sure this is my misunderstanding, but I still don't understand why Flow type checking wouldn't be forcing storedSubscriptions to be Array<Subscription> and thus require the latest property.

(Going to merge anyway, but if you know the answer would appreciate the education :) )

Show outdated Hide outdated src/renderer/constants/action_types.js
// There is some mismatch between redux state and db state
// If something is in the db, but not in redux, add it to redux
// If something is in redux, but not in the db, add it to the db

This comment has been minimized.

@kauffj

kauffj May 10, 2018

Member

I think this is the most reasonable choice and that we can't (or shouldn't) do anything differently immediately, but I also think it's guaranteed that this will cause a bug or confusion in the future.

@kauffj

kauffj May 10, 2018

Member

I think this is the most reasonable choice and that we can't (or shouldn't) do anything differently immediately, but I also think it's guaranteed that this will cause a bug or confusion in the future.

@lbry-bot lbry-bot assigned seanyesmunt and unassigned kauffj and seanyesmunt May 10, 2018

@seanyesmunt seanyesmunt requested a review from kauffj May 10, 2018

@lbry-bot lbry-bot assigned kauffj and unassigned kauffj May 10, 2018

feature: use internal-apis for subscriptions and add page loader
update subscription types

update changelog

Simplify subscriptions sync logic

add claim type

use let over const

change spinner color based on theme

clean up subscriptions

@kauffj kauffj merged commit 492b160 into master May 15, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@seanyesmunt seanyesmunt deleted the internal-subscriptions branch May 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment