Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'renderOrder' property to schema to decide in which order components render #684

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add 'renderOrder' property to schema to decide in which order components render #684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3a945c
Add 'renderOrder' property to schema to decide in which order compone…
Apr 3, 2020
27c5ccd
Fix resolveComponentReferences to always return bluebird promise
Apr 3, 2020
a3a55ac
Remove resolveComponentReferences/composePage breaking changes
Apr 3, 2020
08b09eb
Make instance filters check for valid objects
Apr 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/render.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ function formDataFromLayout(locals, uri) {
* @return {Promise}
*/
function formDataForRenderer(unresolvedData, { _layoutRef, _ref }, locals) {
return composer.resolveComponentReferences(unresolvedData, locals)
return composer.resolveComponentReferences(unresolvedData, locals, composer.referenceProperty, _ref)
.then((data) => ({
data,
options: { locals, _ref, _layoutRef }
Expand Down
4 changes: 3 additions & 1 deletion lib/routes/_components.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ function getComposed(uri, locals) {
return controller.get(uri, locals).then(function (data) {
// TODO: Check that we can't just reference the composer
// require here because otherwise it's a circular dependency (via html-composer)
return require('../services/composer').resolveComponentReferences(data, locals);
const composer = require('../services/composer');

return composer.resolveComponentReferences(data, locals, composer.referenceProperty, uri);
});
}

Expand Down
4 changes: 3 additions & 1 deletion lib/routes/_layouts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ function getComposed(uri, locals) {
return controller.get(uri, locals).then(data => {
// TODO: Check that we can't just reference the composer
// require here because otherwise it's a circular dependency (via html-composer)
return require('../services/composer').resolveComponentReferences(data, locals);
const composer = require('../services/composer');

return composer.resolveComponentReferences(data, locals, composer.referenceProperty, uri);
});
}

Expand Down
2 changes: 1 addition & 1 deletion lib/services/components.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ function publish(uri, data, locals) {
}

return get(replaceVersion(uri), locals)
.then(latestData => composer.resolveComponentReferences(latestData, locals, composer.filterBaseInstanceReferences))
.then(latestData => composer.resolveComponentReferences(latestData, locals, composer.filterBaseInstanceReferences, uri))
.then(versionedData => dbOps.cascadingPut(put)(uri, versionedData, locals));
}

Expand Down
101 changes: 76 additions & 25 deletions lib/services/composer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ const _ = require('lodash'),
components = require('./components'),
references = require('./references'),
mapLayoutToPageData = require('../utils/layout-to-page-data'),
referenceProperty = '_ref';
{ getSchema } = require('../utils/schema'),
{ isLayout, isComponent } = require('clayutils'),
referenceProperty = '_ref',
renderOrderProperty = 'renderOrder',
defaultRenderOrder = 1;
var log = require('./logger').setup({ file: __filename });

/**
Expand All @@ -14,35 +18,80 @@ var log = require('./logger').setup({ file: __filename });
* @param {object} data
* @param {object} locals Extra data that some GETs use
* @param {function|string} [filter='_ref']
* @param {string} [uri]
* @returns {Promise} - Resolves with composed component data
*/
function resolveComponentReferences(data, locals, filter = referenceProperty) {
const referenceObjects = references.listDeepObjects(data, filter);

return bluebird.all(referenceObjects).each(referenceObject => {
return components.get(referenceObject[referenceProperty], locals)
.then(obj => {
// the thing we got back might have its own references
return resolveComponentReferences(obj, locals, filter).finally(() => {
_.assign(referenceObject, _.omit(obj, referenceProperty));
}).catch(function (error) {
const logObj = {
stack: error.stack,
cmpt: referenceObject[referenceProperty]
};

if (error.status) {
logObj.status = error.status;
}

log('error', `${error.message}`, logObj);

return bluebird.reject(error);
function resolveComponentReferences(data, locals, filter = referenceProperty, uri) {
const referenceObjects = references.listDeepValuesByKey(data, filter),
schemaPromise = uri && Object.keys(referenceObjects).length && (isComponent(uri) || isLayout(uri)) ? getSchema(uri) : bluebird.resolve();

return schemaPromise.then(schema => {
const orderedRefs = orderReferences(referenceObjects, schema);

return bluebird.each(orderedRefs, ([, referenceObject]) => {
const ref = referenceObject[referenceProperty];

return components.get(ref, locals)
.then(obj => {
// the thing we got back might have its own references
return resolveComponentReferences(obj, locals, filter, ref).finally(() => {
_.assign(referenceObject, _.omit(obj, referenceProperty));
}).catch(function (error) {
const logObj = {
stack: error.stack,
cmpt: referenceObject[referenceProperty]
};

if (error.status) {
logObj.status = error.status;
}

log('error', `${error.message}`, logObj);

return bluebird.reject(error);
});
});
});
});
}).then(() => data);
}

/**
* Orders deep references based on the parent's schema.
* @param {object} referenceObjects
* @param {object} schema
* @returns {array}
*/
function orderReferences(referenceObjects, schema) {
const pairedRefs = _.toPairs(referenceObjects);

if (schema) {
// group and sort by orders from the schema
return _.chain(pairedRefs)
.groupBy(([path]) => {
const splitPath = path.split('.');

// remove array indexes
if (!isNaN(splitPath[splitPath.length - 1])) {
splitPath.pop();
}

return [
_.get(schema, [...splitPath, '_componentList', renderOrderProperty]),
_.get(schema, [...splitPath, '_component', renderOrderProperty]),
defaultRenderOrder
].find(order => _.isNumber(order));
})
.toPairs()
.sortBy(a => a[0])
.map(a => a[1])
.flatten()
.value();
} else {
// if there's no schema, return original refs
return pairedRefs;
}
}

/**
* Compose a page, recursively filling in all component references with
* instance data.
Expand All @@ -65,12 +114,14 @@ function composePage(pageData, locals) {
* @returns {Boolean}
*/
function filterBaseInstanceReferences(obj) {
return _.isString(obj[referenceProperty]) && obj[referenceProperty].indexOf('/instances/') !== -1;
return _.isObject(obj) && _.isString(obj[referenceProperty]) && obj[referenceProperty].indexOf('/instances/') !== -1;
}

module.exports.resolveComponentReferences = resolveComponentReferences;
module.exports.composePage = composePage;
module.exports.filterBaseInstanceReferences = filterBaseInstanceReferences;
module.exports.referenceProperty = referenceProperty;
Copy link
Author

Choose a reason for hiding this comment

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

had to export this to use it for calling resolveComponentReferences in order to remove the breaking change for that call signature


// For testing
module.exports.setLog = (fakeLogger) => log = fakeLogger;
module.exports.orderReferences = orderReferences;
40 changes: 40 additions & 0 deletions lib/services/composer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,4 +200,44 @@ describe(_.startCase(filename), function () {
expect(fn({ bar: 'foo' })).to.be.false;
});
});

describe('orderReferences', function () {
const fn = lib[this.title],
referenceObjects = {
someComponent: { _ref: 'ref1' },
anotherComponent: { _ref: 'ref2' },
'componentList.0': { _ref: 'ref3' },
'componentList.1': { _ref: 'ref4' }
};

it('maintains order without a schema', function () {
const result = fn(referenceObjects);

expect(result).to.have.deep.property('0.0', 'someComponent');
expect(result).to.have.deep.property('1.0', 'anotherComponent');
expect(result).to.have.deep.property('2.0', 'componentList.0');
expect(result).to.have.deep.property('3.0', 'componentList.1');
Copy link

Choose a reason for hiding this comment

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

fyi this can be rewritten to

expect(result).to.nested.include({
  '0.0': 'someComponent'
  '1.0': 'anotherComponent'
  '2.0': 'componentList.0'
  '3.0': 'componentList.1'
})

docs

});

it('orders based on schema', function () {
const schema = {
someComponent: {
_component: {
renderOrder: 3
}
},
componentList: {
_componentList: {
renderOrder: 2
}
}
},
result = fn(referenceObjects, schema);

expect(result).to.have.deep.property('0.0', 'anotherComponent');
expect(result).to.have.deep.property('1.0', 'componentList.0');
expect(result).to.have.deep.property('2.0', 'componentList.1');
expect(result).to.have.deep.property('3.0', 'someComponent');
});
});
});
2 changes: 1 addition & 1 deletion lib/services/layouts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ function publish(uri, data, locals) {
}

return get(replaceVersion(uri), locals)
.then(latestData => composer.resolveComponentReferences(latestData, locals, composer.filterBaseInstanceReferences))
.then(latestData => composer.resolveComponentReferences(latestData, locals, composer.filterBaseInstanceReferences, uri))
.then(versionedData => dbOps.cascadingPut(put)(uri, versionedData, locals))
.then(data => meta.publishLayout(uri, user).then(() => {
bus.publish('publishLayout', { uri, data, user });
Expand Down
6 changes: 3 additions & 3 deletions lib/services/pages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ function addOp(uri, data, ops) {
* @returns {boolean}
*/
function isInstanceReferenceObject(data) {
return _.isString(data._ref) && data._ref.indexOf('/instances/') > -1;
return _.isObject(data) && _.isString(data._ref) && data._ref.indexOf('/instances/') > -1;
}

/**
Expand All @@ -72,7 +72,7 @@ function getPageClonePutOperations(pageData, locals) {
// for all strings that are component references
promises.push(components.get(pageValue, locals)
// only follow the paths of instance references. Don't clone default components
.then(refData => composer.resolveComponentReferences(refData, locals, isInstanceReferenceObject))
.then(refData => composer.resolveComponentReferences(refData, locals, isInstanceReferenceObject, pageValue))
.then(resolvedData => {
// for each instance reference within resolved data
_.each(references.listDeepObjects(resolvedData, isInstanceReferenceObject), obj => {
Expand Down Expand Up @@ -149,7 +149,7 @@ function getRecursivePublishedPutOperations(locals) {
* 4) Get list of put operations needed
*/
return components.get(rootComponentRef, locals)
.then(data => composer.resolveComponentReferences(data, locals))
.then(data => composer.resolveComponentReferences(data, locals, composer.referenceProperty, rootComponentRef))
.then(data => dbOps.getPutOperations(components.cmptPut, replaceVersion(rootComponentRef, 'published'), data, locals));
};
}
Expand Down
26 changes: 26 additions & 0 deletions lib/services/references.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,31 @@ function listDeepObjects(obj, filter) {
return list;
}

/**
* Search through an object and find all deep key-value pairs matching a filter.
* @param {object} obj
* @param {Function} [filter=_.identity] Optional filter
Copy link

@olsonpm olsonpm Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So actually since this is only called by resolveComponentReferences and that takes a {function|string}, this should do the same.

I would just put * in for the filter. For one, strings are passed here, but really anything that's accepted by lodash's _.filter which is just about everything can be passed here.

Copy link
Author

Choose a reason for hiding this comment

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

sure. was just keeping the same definition as the listDeepObjects function

* @param {array} [path] Path of this value
* @returns {object}
*/
function listDeepValuesByKey(obj, filter = _.identity, path = []) {
Copy link

Choose a reason for hiding this comment

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

Unfortunately the order listDeepValuesByKey results in is different from listDeepObjects due to your use of recursion (depth vs breadth first).

I tested on an article - and this matters because if you change the default order then when people update their version and rely on a certain rendering order, their code will behave differently and potentially break.

Copy link
Author

Choose a reason for hiding this comment

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

hey phil. tested on an article as well. the only thing that would be affected here are refs that are properties, which we already know order shouldn't be relied upon for those. array ordering is still maintained unless there is a ref inside of a ref (which afaik would never happen before data is populated)

Copy link

Choose a reason for hiding this comment

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

i'll get an example to show what i mean

Copy link

@olsonpm olsonpm Apr 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

so testing against this article

listDeepObjects returns with the order of these refs
whereas listDeepValuesByKey returns with this order

In terms of keeping the public api of amphora stable, listDeepValuesByKey needs to preserve the ordering by default, regardless whether the order should be relied upon.

For instance, we were relying upon the rendering order and if that changed with a minor update, that would be a frustrating bug to find.

Ultimately it's up to the lib author to define the api and what constitutes semver changes, so if you disagree then we can let them decide.

let result = {};

if (_.isObject(obj)) {
// loop through any objects or arrays
result = _.reduce(obj, (cumm, curr, key) => _.assign(cumm, listDeepValuesByKey(curr, filter, [...path, key])), {});
}

if (_.iteratee(filter)(obj) && path.length) {
Copy link

@olsonpm olsonpm Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

why use iteratee here instead of _.filter(obj, filter) ?

I was wrong about _.filter because that returns an array no matter what. However I would still suggest doing something to make this more readable because _.iteratee is not a common lodash function nor is it obvious what it's doing here. So if you use it then do something like this

const passesFilter = _.iteratee(filter)

if (path.length && passesFilter(obj)) { ... }

Or more preferably separate out the function and string cases to make it more clear

const passesFilter = typeof filter === 'function'
  ? filter
  : obj => _.get(obj, filter)

// add the key to results if it matches the filter
const key = path.join('.');

result[key] = obj;
}

return result;
}

/**
* @param {string} version
* @returns {function}
Expand Down Expand Up @@ -155,3 +180,4 @@ module.exports.urlToUri = urlToUri;
module.exports.omitPageConfiguration = omitPageConfiguration;
module.exports.getPageReferences = getPageReferences;
module.exports.listDeepObjects = listDeepObjects;
module.exports.listDeepValuesByKey = listDeepValuesByKey;
43 changes: 43 additions & 0 deletions lib/services/references.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,4 +170,47 @@ describe(_.startCase(filename), function () {
]);
});
});

describe('listDeepValuesByKey', function () {
const fn = lib[this.title];

it('listDeepValuesByKey gets all deep values by key', function () {
const result = fn({a:{b:{c:{d:'e'}}, f:{g:{h:'e'}}}, abc:[6,{p:2}]});

expect(result).to.deep.equal({
a: {b:{c:{d:'e'}}, f:{g:{h:'e'}}},
'a.b': {c:{d:'e'}},
'a.b.c': {d:'e'},
'a.b.c.d': 'e',
'a.f': {g:{h:'e'}},
'a.f.g': {h:'e'},
'a.f.g.h': 'e',
abc: [6,{p:2}],
'abc.0': 6,
'abc.1': {p:2},
'abc.1.p': 2
});
});

it('listDeepValuesByKey gets all deep objects using filter', function () {
const result = fn({a:{b:{c:{d:'e'}}, f:{g:{h:'e'}}}}, _.isObject);

expect(Object.values(result)).to.have.length(5);
});

it('listDeepValuesByKey can filter by existence of properties', function () {
const result = fn({a:{b:{c:{d:'e'}}, f:{d:{g:'e'}}}}, 'd');

expect(Object.values(result)).to.have.length(2);
});

it('listDeepValuesByKey can filter by component', function () {
const result = fn({a: {type:'yarn'}, b: {c: {type:'sweater'}}}, function (obj) { return !!obj.type; });

expect(result).to.deep.equal({
a: { type: 'yarn' },
'b.c': { type: 'sweater' }
});
});
});
});