Show request refactor #277

2017-11-30T00:42:00+01:00

bones7242 commented

2017-11-30 00:42:00 +01:00

(Migrated from github.com)

refactored serve/show routes
show route returns show page immediately with content metadata
show page updates user as it searches for content that is not held locally

- refactored serve/show routes - show route returns show page immediately with content metadata - show page updates user as it searches for content that is not held locally

kauffj (Migrated from github.com) reviewed 2017-11-30 00:42:00 +01:00

bones7242 commented

2017-12-02 00:32:05 +01:00

(Migrated from github.com)

changed base to development to follow GitFlow branching model

changed base to `development` to follow GitFlow branching model

kauffj (Migrated from github.com) requested changes 2017-12-05 00:38:25 +01:00

kauffj (Migrated from github.com) left a comment

Nice work! Here's nits I picked.

README.md

					
				@ -24,19 +24,23 @@ spee.ch is a single-serving site that reads and publishes images and videos to a

				## API

kauffj (Migrated from github.com) commented

2017-12-05 00:08:38 +01:00

Hooray capitalization consistency 💯

Hooray capitalization consistency :100:

controllers/serveController.js

kauffj (Migrated from github.com) commented

2017-12-05 00:09:35 +01:00

channelId is still a claim id. I might call this something like channelClaimId to make that clearer.

`channelId` is still a claim id. I might call this something like `channelClaimId` to make that clearer.

kauffj (Migrated from github.com) commented

2017-12-05 00:10:41 +01:00

No need to wrap resolve and reject if you are just passing them the same parameter. You can just do:

db.Claim.getLongClaimId(claimName, claimId).then(resolve, reject)

No need to wrap `resolve` and `reject` if you are just passing them the same parameter. You can just do: `db.Claim.getLongClaimId(claimName, claimId).then(resolve, reject)`

kauffj (Migrated from github.com) commented

2017-12-05 00:11:33 +01:00

(Same comment as above - unnecessary wrappers)

controllers/serveController.js

					
				@ -253,0 +86,4 @@

				        }

				        return file.dataValues;

				      });

				  },

kauffj (Migrated from github.com) commented

2017-12-05 00:14:41 +01:00

I'd look at whether this trimming can be done in the model layer, rather than the controller (i.e. can we perform the TRIM in SQL or at least before the data makes it out of the model layer).

If you don't have to trim here, you can then just do things like:

const thumb = claimInfo.thumbnail || DEFAULT_THUMBNAIL

which is a lot cleaner and possibly obviates the need for the function entirely

I'd look at whether this trimming can be done in the model layer, rather than the controller (i.e. can we perform the TRIM in SQL or at least before the data makes it out of the model layer). If you don't have to trim here, you can then just do things like: `const thumb = claimInfo.thumbnail || DEFAULT_THUMBNAIL` which is a lot cleaner and possibly obviates the need for the function entirely

controllers/serveController.js

kauffj (Migrated from github.com) commented

2017-12-05 00:22:30 +01:00

Is there a reason we don't just use something like null to mean no record found? This goes for here and a few other places.

Is there a reason we don't just use something like `null` to mean no record found? This goes for here and a few other places.

helpers/handlebarsHelpers.js

					
				@ -5,3 +8,4 @@

				  },

				  googleAnalytics () {

				    const googleApiKey = config.analytics.googleId;

				    const gaCode = `<script>(function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){

kauffj (Migrated from github.com) commented

2017-12-05 00:16:21 +01:00

Same with this - let's sanitize data sooner. Ideally, the data could be sanitized in both cases before it even makes it into the DB. Otherwise, you should clean it as close as you can to where it is retrieved. It'll make your code easier to write and will prevent introduction of bugs caused by forgetting to do a trim check.

Same with this - let's sanitize data sooner. Ideally, the data could be sanitized in both cases _before_ it even makes it into the DB. Otherwise, you should clean it as close as you can to where it is retrieved. It'll make your code easier to write and will prevent introduction of bugs caused by forgetting to do a trim check.

helpers/sequelizeHelpers.js

					
				@ -1,23 +1,23 @@

				module.exports = {

				  returnShortId: function (result, longId) {

				  returnShortId: function (claimsArray, longId) {

				    let claimIndex;

kauffj (Migrated from github.com) commented

2017-12-05 00:17:42 +01:00

👍 I love better names

:+1: I love better names

models/certificate.js

					
				@ -171,12 +170,29 @@ module.exports = (sequelize, { STRING, BOOLEAN, INTEGER, TEXT, DECIMAL }) => {

				    });

kauffj (Migrated from github.com) commented

2017-12-05 00:24:42 +01:00

Since 40 is used as a magic/special value both inside of this function and elsewhere, it should be a constant somewhere. Probably on the Claim class?

Since 40 is used as a magic/special value both inside of this function and elsewhere, it should be a constant somewhere. Probably on the `Claim` class?

models/index.js

					
				@ -69,11 +69,12 @@ db.upsert = (Model, values, condition, tableName) => {

				    })

				    .catch(function (error) {

				      logger.error(`${tableName}.upsert error`, error);

				      throw error;

kauffj (Migrated from github.com) commented

2017-12-05 00:26:09 +01:00

This one seems important :)

public/assets/js/assetConstructor.js

kauffj (Migrated from github.com) commented

2017-12-05 00:27:10 +01:00

I'd like to discuss this design and the purpose of it some in person. Please message me on chat and we'll get on a Hangout.

kauffj (Migrated from github.com) commented

2017-12-05 00:28:21 +01:00

This is fine, but it does look like for the browsers we're targeting that we can use fetch. This is a much nicer API than XHR. See: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API

This is fine, but it does look like for the browsers we're targeting that we can use `fetch`. This is a much nicer API than XHR. See: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API

routes/api-routes.js

kauffj (Migrated from github.com) commented

2017-12-05 00:29:49 +01:00

This is only used in one place? Possibly doesn't need to be a function? (Not sure, fine either way)

routes/serve-routes.js

kauffj (Migrated from github.com) commented

2017-12-05 00:33:06 +01:00

You may want to look at using lbryuri.js for some of this: https://github.com/lbryio/lbry-app/blob/master/src/renderer/js/lbryuri.js

In particular, the parse and build functions (which I recommend just trying to call so you see what they do if reading the code is tricky).

You may want to look at using `lbryuri.js` for some of this: https://github.com/lbryio/lbry-app/blob/master/src/renderer/js/lbryuri.js In particular, the `parse` and `build` functions (which I recommend just trying to call so you see what they do if reading the code is tricky).

routes/serve-routes.js

					
				@ -34,0 +59,4 @@

				    responseType = SHOW;

				    if (!clientAcceptsHtml(headers)) {  // this is in case someone embeds a show url

				      responseType = SERVE;

				    }

kauffj (Migrated from github.com) commented

2017-12-05 00:34:22 +01:00

Should all of this logic be in routes itself? (I don't actually know Express paradigms)

Should all of this logic be in `routes` itself? (I don't actually know Express paradigms)

routes/serve-routes.js

					
				@ -34,0 +90,4 @@

				      });

				}

				function serveAssetToClient (claimId, name, res) {

kauffj (Migrated from github.com) commented

2017-12-05 00:34:48 +01:00

DRY (just put it in a function, it's fine if it's short)

views/layouts/showlite.handlebars

					
				@ -15,1 +6,3 @@

					{{/unless}}

				    {{#unless claimInfo.nsfw}}

				        {{{addTwitterCard claimInfo }}}

				        {{{addOpenGraph claimInfo }}}

kauffj (Migrated from github.com) commented

2017-12-05 00:21:33 +01:00

Is it possible for showlite and show (and possibly other layouts) to share more of their common logic (particularly <head>), inside of a shared template?

Is it possible for `showlite` and `show` (and possibly other layouts) to share more of their common logic (particularly `<head>`), inside of a shared template?

views/partials/asset.handlebars

kauffj (Migrated from github.com) commented

2017-12-05 00:37:05 +01:00

Among the questions we'll discuss: why not just...

const asset = new Asset({
  src: "foo",
  claimId: "bar",
  ...
})

Among the questions we'll discuss: why not just... ``` const asset = new Asset({ src: "foo", claimId: "bar", ... }) ```

kauffj (Migrated from github.com) commented

2017-12-05 00:37:53 +01:00

Also, given that asset is used elsewhere in code, we might want this class to have a more specific name.

bones7242 (Migrated from github.com) reviewed 2017-12-06 02:42:48 +01:00

controllers/serveController.js

bones7242 (Migrated from github.com) commented

2017-12-06 02:42:48 +01:00

I'm elevating this out of the model layer, but keeping at the controller level

bones7242 (Migrated from github.com) reviewed 2017-12-07 00:20:32 +01:00

routes/api-routes.js

bones7242 (Migrated from github.com) commented

2017-12-07 00:20:31 +01:00

yeah, only used in one place at the moment (and perhaps something that should happen lower in the model layer), but I am trying to compartmentalize code into functions where possible for ease of reading as well as testing.

bones7242 (Migrated from github.com) reviewed 2017-12-07 03:51:14 +01:00

routes/serve-routes.js

bones7242 (Migrated from github.com) commented

2017-12-07 03:51:14 +01:00

this was very helpful. I made my own version since the uri on spee.ch is slightly different, but used this pattern. Probably could still be cleaned up a bit, but it simplified the code a lot.

bones7242 (Migrated from github.com) reviewed 2017-12-07 21:24:04 +01:00

routes/serve-routes.js

					
				@ -34,0 +59,4 @@

				    responseType = SHOW;

				    if (!clientAcceptsHtml(headers)) {  // this is in case someone embeds a show url

				      responseType = SERVE;

				    }

bones7242 (Migrated from github.com) commented

2017-12-07 21:24:04 +01:00

Agreed. I moved it to the controller layer.

bones7242 (Migrated from github.com) reviewed 2017-12-07 21:35:57 +01:00

views/layouts/showlite.handlebars

					
				@ -15,1 +6,3 @@

					{{/unless}}

				    {{#unless claimInfo.nsfw}}

				        {{{addTwitterCard claimInfo }}}

				        {{{addOpenGraph claimInfo }}}

bones7242 (Migrated from github.com) commented

2017-12-07 21:35:57 +01:00

There may be more room for synergy, but for now I condensed the common header tags into a handlebars helper and tightened up the open graph/twitter tag helper.

Sign in to join this conversation.

No reviewers

kauffj

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/spee.ch#277

				`@ -24,19 +24,23 @@ spee.ch is a single-serving site that reads and publishes images and videos to a`
				`## API`

				`@ -171,12 +170,29 @@ module.exports = (sequelize, { STRING, BOOLEAN, INTEGER, TEXT, DECIMAL }) => {`
				`});`

Rows
Columns