Skip to content

Conversation

@nickbalestra
Copy link
Contributor

Context: this will allow for example for React SSR to delegate to the Registry for the rendering, in case the template is not supported by the client.

https://opentable.atlassian.net/browse/OC-93

@nickbalestra nickbalestra changed the title [OC-93] - Send supported templates as http header [OC-93][WIP] - Send supported templates as http header Aug 11, 2017
@nickbalestra nickbalestra changed the title [OC-93][WIP] - Send supported templates as http header [OC-93] - Send supported templates as http header Aug 11, 2017
@nickbalestra nickbalestra changed the title [OC-93] - Send supported templates as http header [OC-93][wip] - Send supported templates as http header Aug 11, 2017
@nickbalestra
Copy link
Contributor Author

To check for the warmup/init - it seems to me that we don't do any options sanitizing (neither here or here - only config sanit here.

@mattiaerre I want therefore to add options sanitization within the warmup as well, what you think?

Copy link
Member

@mattiaerre mattiaerre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

over to you @nickbalestra


const sanitiseDefaultOptions = function(options) {
const getTemplatesInfo = templates =>
templates.reduce((templatesHash, template) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

me ❤️ this

return templatesHash;
}, {});

const sanitiseDefaultOptions = function(options, config) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to avoid param reassign; shall we Object.assign({}, options)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking more about something like this:

const sanitiseDefaultOptions = (options, config) => {
  let copy = Object.assign({}, options);
  if (_.isFunction(copy)) {
    copy = {};
  }

  copy.headers = lowerHeaderKeys(copy.headers);
  copy.headers['user-agent'] =
    copy.headers['user-agent'] || getDefaultUserAgent();
  copy.headers.templates =
    copy.headers.templates || getTemplatesInfo(config.templates);

  copy.timeout = copy.timeout || 5;
  return copy;
};

src/sanitiser.js Outdated
conf.components = conf.components || {};
conf.cache = conf.cache || {};
conf.templates = conf.templates
? _.uniq(conf.templates.concat(baseTemplates))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to have a test that covers the uniq method 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree, btw I've added coverage for the uniq helper but we don't have any test at all for all the rest of the utils, to be address in an had hoc PR.

@nickbalestra nickbalestra changed the title [OC-93][wip] - Send supported templates as http header [OC-93] - Send supported templates as http header Aug 12, 2017
Copy link
Member

@mattiaerre mattiaerre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added another comment for you @nickbalestra

return templatesHash;
}, {});

const sanitiseDefaultOptions = function(options, config) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking more about something like this:

const sanitiseDefaultOptions = (options, config) => {
  let copy = Object.assign({}, options);
  if (_.isFunction(copy)) {
    copy = {};
  }

  copy.headers = lowerHeaderKeys(copy.headers);
  copy.headers['user-agent'] =
    copy.headers['user-agent'] || getDefaultUserAgent();
  copy.headers.templates =
    copy.headers.templates || getTemplatesInfo(config.templates);

  copy.timeout = copy.timeout || 5;
  return copy;
};

@nickbalestra
Copy link
Contributor Author

@mattiaerre yes, much better so

Copy link
Member

@mattiaerre mattiaerre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this LGTM I also added some additional comments for you @nickbalestra but very minor stuff. you can automerge if you like 😉


const sanitiseDefaultOptions = function(options, config) {
if (_.isFunction(options)) {
options = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be part of the "copy" path as we are changing input if we do that,
but we can iterate later

uniques = uniques.concat(element);
}
}
return uniques;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about this alternative solution?

const uniq = (input) => {
  const obj = input.reduce((accumulator, element) => {
    const copy = Object.assign({}, accumulator);
    copy[element] = element;
    return copy;
  }, {});

  return Object.keys(obj).map(key => obj[key]);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imho there is no point in copying the accumulator each time.
But the Object.keys works, I'll take that :)

@nickbalestra
Copy link
Contributor Author

@mattiaerre everything should be addresses

Copy link
Member

@mattiaerre mattiaerre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very good thanks for that @nickbalestra

'5',
'6',
'a',
'b'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the nice side effect is that is is also "sorting" 😊

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants