prepare-server-get-component-parameters

[mattiaerre]

Description

• add mergeObjects function to build the query string based on component and options parameters

• fix tests

• add test:unit as npm script

[mattiaerre]

this PR should fix #431 please @gtrias can you have a look at is and see if this makes sense to you?

// cc @matteofigus @ivan-dimitrov-skyscanner

[matteofigus]

Generally speaking this PR flips the coin so that component.parameters are used over options.parameters, which may be a common scenario for when using renderComponents with a single component, but still just one of the two ways to use parameters - which can actually be used together.

I would like a test for a request like

client.renderComponents([{
  name: 'my-component',
  parameters: { a:1, b: 2 }
}], { parameters: { b: 3, c: 4 }, callback);

That would test url would be my-component?a=1&b=2&c=4.

[mattiaerre]

@matteofigus I've added a new function that merges 2 objects together and tests that should cover the scenario you've described; let me know what you think.

// @gtrias @ivan-dimitrov-skyscanner

[gtrias]

@mattiaerre I'm going to test it right now. Thanks a lot for the quick fix ;)

[gtrias]

Hey @mattiaerre how can I try your branch?
I'm trying to add "oc-client": "git+ssh://git@github.com:opentable/oc.git#prepare-server-get-component-parameters" and running rm -rf node_modules/oc-client && npm install.
But then it is installing all oc repository and nodejs is unable to find module oc-client:

Error: Cannot find module 'oc-client'

Is there a way to specify the client folder? AFAIK npm doesn't allow to do that npm/npm#2974

[matteofigus]

what about using _.extend instead of mergeObjects. We use it a lot everywhere so it would be more consistent.

var _ = require('underscore')
var p = _.extend(options.parameters, component.parameters)
// example
_.extend({b:3, c:4}, {a:1, b:2})
// => { b: 2, c: 4, a: 1 }

[mattiaerre]

I was thinking about that but looking at the package.json I couldn't see lodash as a dependency (https://github.com/opentable/oc/blob/master/client/package.json#L17-L21) also in ES6 this will be part of the language so I preferred not to add a dependency and go "vanilla". plus the component is a standalone one (and fully tested) so we can always swap its internal implementation.

[matteofigus]

Can we include a common parameter to ensure our merge goes in the right way, such as { message: 'hello', p1: 'v3'} and be sure qs has ...&p1=v1

[mattiaerre]

this is covered at the mergeObjects level but I will add a specific test for this scenario. and in this case, p1=v3 as the component's parameters win over the option ones (as per your other example above).

[mattiaerre]

over to you @matteofigus

[gtrias]

@mattiaerre now it works my use case :). But I haven't tried all the scenarios btw...

[gtrias]

I can confirm using global parameters works as well but they can be overridden by the component ones...

[matteofigus]

Awesome stuff thanks @mattiaerre

[matteofigus]

Just published. You should be able to upgrade and test now @gtrias thanks for your help!