465 social share with extensions #492

Merged
bones7242 merged 10 commits from 465-social-share-with-extensions into master 2018-06-27 18:51:51 +02:00
bones7242 commented 2018-06-26 08:48:02 +02:00 (Migrated from github.com)

adds open graph support for image urls with the file type suffix. e.g. spee.ch/a/example.jpg

adds open graph support for image urls with the file type suffix. e.g. spee.ch/a/example.jpg
skhameneh (Migrated from github.com) reviewed 2018-06-26 08:48:02 +02:00
neb-b (Migrated from github.com) reviewed 2018-06-26 22:56:05 +02:00
neb-b (Migrated from github.com) left a comment

Will the route changing affect previously shared URLs?

Will the route changing affect previously shared URLs?
@ -0,0 +1,54 @@
const logger = require('winston');
const { EMBED, BROWSER, SOCIAL } = require('../constants/request_types.js');
function headersMatchesSocialBotList (headers) {
neb-b (Migrated from github.com) commented 2018-06-26 22:53:39 +02:00

This could be simplified by using an object instead of an array

const socialBotList = {
    'facebookexternalhit': 1,
    'Twitterbot': 1,
}
​
if (socialBotList[userAgent]) {
  logger.debug();
  return true;
} else {
  return false;
}
This could be simplified by using an object instead of an array ``` const socialBotList = { 'facebookexternalhit': 1, 'Twitterbot': 1, } ​ if (socialBotList[userAgent]) { logger.debug(); return true; } else { return false; } ```
bones7242 (Migrated from github.com) reviewed 2018-06-27 18:49:35 +02:00
@ -0,0 +1,54 @@
const logger = require('winston');
const { EMBED, BROWSER, SOCIAL } = require('../constants/request_types.js');
function headersMatchesSocialBotList (headers) {
bones7242 (Migrated from github.com) commented 2018-06-27 18:49:35 +02:00

cool pattern. I updated to use this.

cool pattern. I updated to use this.
bones7242 commented 2018-06-27 18:51:37 +02:00 (Migrated from github.com)

@seanyesmunt previous URLs should not be negatively affected. This adds a new route, but does not remove the existing routes. The new route will be used only for the image sources for SEO tags, so it is specifically for serving images for SEO.

@seanyesmunt previous URLs should not be negatively affected. This adds a new route, but does not remove the existing routes. The new route will be used only for the image sources for SEO tags, so it is specifically for serving images for SEO.
kauffj (Migrated from github.com) reviewed 2018-07-03 20:12:24 +02:00
@ -0,0 +32,4 @@
return SOCIAL;
}
// if request is not from a social bot...
if (hasFileExtension) {
kauffj (Migrated from github.com) commented 2018-07-03 20:12:24 +02:00

Multiple return statements in a function can be a contentious topic (e.g. https://stackoverflow.com/questions/36707/should-a-function-have-only-one-return-statement), but I think a function like this is better if the returns are just inlined, rather than setting a variable.

Multiple return statements in a function can be a contentious topic (e.g. https://stackoverflow.com/questions/36707/should-a-function-have-only-one-return-statement), but I think a function like this is better if the returns are just inlined, rather than setting a variable.
kauffj (Migrated from github.com) reviewed 2018-07-03 20:13:16 +02:00
@ -0,0 +1,54 @@
const logger = require('winston');
const { EMBED, BROWSER, SOCIAL } = require('../constants/request_types.js');
function headersMatchesSocialBotList (headers) {
kauffj (Migrated from github.com) commented 2018-07-03 20:13:16 +02:00

@seanyesmunt do you prefer that to just:

const haystack = ['a', 'b']
if (haystack.includes(needle))
{
  ...
}
@seanyesmunt do you prefer that to just: ``` const haystack = ['a', 'b'] if (haystack.includes(needle)) { ... } ```
kauffj (Migrated from github.com) reviewed 2018-07-03 20:14:31 +02:00
@ -0,0 +1,54 @@
const logger = require('winston');
const { EMBED, BROWSER, SOCIAL } = require('../constants/request_types.js');
function headersMatchesSocialBotList (headers) {
kauffj (Migrated from github.com) commented 2018-07-03 20:14:31 +02:00

(technically includes is presumably O(n) and a map lookup O(1), but this is such an efficient operation that this doesn't matter)

(_technically_ `includes` is presumably `O(n)` and a map lookup `O(1)`, but this is such an efficient operation that this doesn't matter)
Sign in to join this conversation.
No reviewers
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#492
No description provided.