use canonical_url for everything #187

2019-08-19T05:57:32+02:00

neb-b commented

2019-08-19 05:57:32 +02:00

(Migrated from github.com)

This populates byUri with claim.canonical_url instead of claim.permanent_url everywhere.

Once both apps are moved over, the search results should move to canonical_url too:
https://github.com/lbryio/lighthouse/issues/163

lbryURI.js

The previous version of buildURI and parseURI did not work properly with canonical_urls
buildURI and parseURI will continue to work the same (which will be unreliable if using claim.canonical_url), but will blow up your console with errors in dev mode if claimId, claimName or contentName is passed in. These should be replaced with {stream}/{channel}Name, {stream}/{channel}ClaimId.

This populates `byUri` with `claim.canonical_url` instead of `claim.permanent_url` everywhere. Once both apps are moved over, the search results should move to canonical_url too: https://github.com/lbryio/lighthouse/issues/163 ### lbryURI.js - The previous version of `buildURI` and `parseURI` did not work properly with canonical_urls - `buildURI` and `parseURI` will continue to work the same (which will be unreliable if using claim.canonical_url), but will blow up your console with errors in dev mode if `claimId`, `claimName` or `contentName` is passed in. These should be replaced with `{stream}/{channel}Name`, `{stream}/{channel}ClaimId`.

akinwale (Migrated from github.com) reviewed 2019-08-19 05:57:32 +02:00

neb-b (Migrated from github.com) reviewed 2019-08-19 06:04:26 +02:00

src/lbryURI.js

					
				@ -57,3 +67,3 @@

				  if (isChannel) {

				  if (includesChannel) {

				    if (!channelName) {

neb-b (Migrated from github.com) commented

2019-08-19 06:04:26 +02:00

Not sure the best way to do this.

Should path include the claimId? Should there be path and pathClaimId?

Not sure the best way to do this. Should `path` include the claimId? Should there be `path` and `pathClaimId`?

kauffj (Migrated from github.com) reviewed 2019-08-20 16:53:33 +02:00

src/lbryURI.js

					
				@ -54,3 +63,2 @@

				  const isChannel = claimName.startsWith('@');

				  const channelName = isChannel ? claimName.slice(1) : claimName;

				  const includesChannel = streamNameOrChannelName.startsWith('@');

kauffj (Migrated from github.com) commented

2019-08-20 16:45:04 +02:00

This could possibly be removed or moved. The function is called parseURI, not parseAndValidateURI. It's one thing to error if invalid characters make it impossible to parse, but I think erroring on a validly structured URI is probably incorrect here.

src/lbryURI.js

					
				@ -57,3 +67,3 @@

				  if (isChannel) {

				  if (includesChannel) {

				    if (!channelName) {

kauffj (Migrated from github.com) commented

2019-08-20 16:49:01 +02:00

According to https://lbry.tech/spec#urls, the path should include the channel name, claim name, and any modifiers. The path is everything between the scheme and any query parameters.

Channel name, claim name, and any modifiers would be sub-properties of the path (and could potentially be returned/parsed out by this function as well).

@lyoshenka, please correct me if I'm wrong about this.

@seanyesmunt we should try to have this function use the same names and standards that the spec specifies. If you think we should do something different than what the spec says, we can also consider amending the spec.

According to https://lbry.tech/spec#urls, the `path` should include the channel name, claim name, and any modifiers. The path is everything between the scheme and any query parameters. Channel name, claim name, and any modifiers would be sub-properties of the path (and could potentially be returned/parsed out by this function as well). @lyoshenka, please correct me if I'm wrong about this. @seanyesmunt we should try to have this function use the same names and standards that the spec specifies. If you think we should do something different than what the spec says, we can also consider amending the spec.

src/redux/actions/claims.js

				`@ -328,10 +328,10 @@ export function doClaimSearch(`

kauffj (Migrated from github.com) commented

2019-08-20 16:49:52 +02:00

why keep two sets of URIs?

src/redux/reducers/claims.js

					
				@ -66,26 +66,25 @@ function handleClaimAction(state: State, action: any): State {

				  const byId = Object.assign({}, state.byId);

				  const channelClaimCounts = Object.assign({}, state.channelClaimCounts);

kauffj (Migrated from github.com) commented

2019-08-20 16:50:34 +02:00

URI or URL? are we consistent with when we use one vs. the other?

src/redux/selectors/claims.js

kauffj (Migrated from github.com) commented

2019-08-20 16:53:21 +02:00

IMO, all of @lbryio/appsquad should be on the same page with the URI type that is promised/implied by default when used without modifiers. Is it the user input URI, the canonical URI, the permanent URI (canonical means something close to permanent so I dislike using both of these as adjectives), or something else?

Then when we have structures like xxxByUri, we understand that it is implicitly promising an index by that URI type, without needing to always specify which type, and specifying only when we deviate from the default.

IMO, all of @lbryio/appsquad should be on the same page with the URI type that is promised/implied by default when used without modifiers. Is it the user input URI, the canonical URI, the permanent URI (canonical means something close to permanent so I dislike using both of these as adjectives), or something else? Then when we have structures like `xxxByUri`, we understand that it is implicitly promising an index by that URI type, without needing to always specify which type, and specifying only when we deviate from the default.

neb-b (Migrated from github.com) reviewed 2019-08-20 18:52:19 +02:00

src/lbryURI.js

					
				@ -54,3 +63,2 @@

				  const isChannel = claimName.startsWith('@');

				  const channelName = isChannel ? claimName.slice(1) : claimName;

				  const includesChannel = streamNameOrChannelName.startsWith('@');

neb-b (Migrated from github.com) commented

2019-08-20 18:52:19 +02:00

I like removing validation from this. We already have isURIValid to validate

I like removing validation from this. We already have `isURIValid` to validate

neb-b commented

2019-08-20 22:13:59 +02:00

(Migrated from github.com)

@kauffj this is ready for another review.

It's a bigger change than I wanted to make but it's already making some of the url code way easier to follow without trying to figure out if a name is for the channel or stream. Let me know what you think.

@kauffj this is ready for another review. It's a bigger change than I wanted to make but it's already making some of the url code way easier to follow without trying to figure out if a name is for the channel or stream. Let me know what you think.

kauffj (Migrated from github.com) approved these changes 2019-08-21 20:56:22 +02:00

kauffj (Migrated from github.com) left a comment

LGTM. If there was a part of this you were specifically concerned about lmk, but it all looked fine to me.

src/redux/actions/search.js

kauffj (Migrated from github.com) commented

2019-08-21 20:55:01 +02:00

should this be a Flow type?

Sign in to join this conversation.

No reviewers

akinwale

kauffj