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.

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

wip: add support for lazily replacing variables #1388

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
63 changes: 63 additions & 0 deletions lib/collection/property.js
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,26 @@ _.assign(Property, /** @lends Property */ {
return Substitutor.box(variables, Substitutor.DEFAULT_VARS).parse(str).toString();
},

/**
* Similar to `replaceSubstitutions` but runs asynchronously
* and supports async value functions
*
* @param {String} str -
* @param {VariableList|Object|Array.<VariableList|Object>} variables -
* @returns {String}
*/
// @todo: improve algorithm via variable replacement caching
replaceSubstitutionsLazy: async function (str, variables) {
// if there is nothing to replace, we move on
if (!(str && _.isString(str))) { return str; }

// if variables object is not an instance of substitutor then ensure that it is an array so that it becomes
// compatible with the constructor arguments for a substitutor
!Substitutor.isInstance(variables) && !_.isArray(variables) && (variables = _.tail(arguments));

return (await Substitutor.box(variables, Substitutor.DEFAULT_VARS).parseLazy(str)).toString();
},

/**
* This function accepts an object followed by a number of variable sources as arguments. One or more variable
* sources can be provided and it will use the one that has the value in left-to-right order.
Expand Down Expand Up @@ -319,6 +339,49 @@ _.assign(Property, /** @lends Property */ {
return _.mergeWith({}, obj, customizer);
},

/**
* Similar to `replaceSubstitutionsIn` but runs asynchronously
* and supports async value functions
*
* @param {Object} obj -
* @param {Array.<VariableList|Object>} variables -
* @returns {Object}
*/
replaceSubstitutionsInLazy: async function (obj, variables) {
// if there is nothing to replace, we move on
if (!(obj && _.isObject(obj))) {
return obj;
}

// convert the variables to a substitutor object (will not reconvert if already substitutor)
variables = Substitutor.box(variables, Substitutor.DEFAULT_VARS);

const promises = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@appurva21 Thoughts on supporting async updates this way? This feels weird but I didn't want to touch the _.mergeWith and customizer fns. Or do you think it's okay if we write a custom implementation for mergeWith which supports async replaces ?

var customizer = function (objectValue, sourceValue, key) {
objectValue = objectValue || {};
if (!_.isString(sourceValue)) {
_.forOwn(sourceValue, function (value, key) {
sourceValue[key] = customizer(objectValue[key], value);
});

return sourceValue;
}

const result = this.replaceSubstitutionsLazy(sourceValue, variables);

promises.push({ key: key, promise: result });

return result;
}.bind(this),
res = _.mergeWith({}, obj, customizer);

await Promise.all(promises.map(async ({ key, promise }) => {
res[key] = await promise;
}));

return res;
},

/**
* This function recursively traverses a variable and detects all instances of variable replacements
* within the string of the object
Expand Down
15 changes: 15 additions & 0 deletions lib/collection/variable.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,20 @@ _.assign(Variable.prototype, /** @lends Variable.prototype */ {
return (!_.isNil(value) && _.isFunction(value.toString)) ? value.toString() : E;
},

/**
* Runs the value function and updates the variable to store
* the return value of the function
*
* @returns {String}
*/
async populate () {
const value = await this.valueOf();

this.valueOf(value);
this.valueType(typeof value);
this.lazy = false;
},

/**
* Typecasts a value to the {@link Variable.types} of this {@link Variable}. Returns the value of the variable
* converted to the type specified in {@link Variable#type}.
Expand Down Expand Up @@ -208,6 +222,7 @@ _.assign(Variable.prototype, /** @lends Variable.prototype */ {
_.has(options, 'system') && (this.system = options.system);
_.has(options, 'disabled') && (this.disabled = options.disabled);
_.has(options, 'description') && (this.describe(options.description));
_.has(options, 'lazy') && (this.lazy = options.lazy);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@appurva21 Thoughts on this approach?
The advantage is that it makes it clear which variables are to be resolved lazily, and which are to be replaced immediately. The disadvantage is that lazy has no meaning for replaceSubstitutions ( non lazy ), and makes the replaceSubstitutionsLazy interface more complicated.

Some other approaches I thought about

  • A) lazily replace all values ( when invoked through replaceSubstitutionsLazy ) but this increases the amount of memory needed, and slightly slows down the function.
  • B) Only lazily replace Promise values. This is a good solution but there is no reliable method of identifying if something is a promise or not. ( Duck typing is not accurate )
  • C) Only lazily replace function values. Could be a good tradeoff but has the same problems as (A), also makes the code slightly less intuitive.

}
});

Expand Down
57 changes: 57 additions & 0 deletions lib/superstring/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ _.assign(SuperString.prototype, /** @lends SuperString.prototype */ {
Substitutor = function (variables, defaults) {
defaults && variables.push(defaults);
this.variables = variables;

// tracks the lazy variables being used
this.lazyVariables = [];
};

_.assign(Substitutor.prototype, /** @lends Substitutor.prototype */ {
Expand Down Expand Up @@ -153,6 +156,54 @@ _.assign(Substitutor.prototype, /** @lends Substitutor.prototype */ {
// }

return value;
},

/**
* @param {SuperString} value -
* @returns {String}
*/
async parseLazy (value) {
// convert the value into a SuperString so that it can return tracking results during replacements
value = new SuperString(value);

// get an instance of a replacer function that would be used to replace ejs like variable replacement
// tokens
var replacer = Substitutor.replacer(this);

// replace the value once and keep on doing it until all tokens are replaced or we have reached a limit of
// replacements
do {
if (this.lazyVariables.length) {
// eslint-disable-next-line no-await-in-loop
await this._populateLazyVariables();
}
value = value.replace(Substitutor.REGEX_EXTRACT_VARS, replacer);
} while (value.replacements && (value.substitutions < Substitutor.VARS_SUBREPLACE_LIMIT));

// @todo: uncomment this code, and try to raise a warning in some way.
// do a final check that if recursion limits are reached then replace with blank string
// if (value.substitutions >= Substitutor.VARS_SUBREPLACE_LIMIT) {
// value = value.replace(Substitutor.REGEX_EXTRACT_VARS, E);
// }

return value.toString();
},

async _populateLazyVariables () {
await Promise.all(this.lazyVariables.map(async (lazyResolution) => {
try {
let r = lazyResolution;

r && _.isFunction(r) && (r = await r());
r && _.isFunction(r.populate) && (await r.populate());
}
catch (e) { /* empty */ }
}));

// Since we've populated the lazy variables,
// they are no longer lazy, so we set lazyVariables tracker
// to empty array
this.lazyVariables = [];
}
});

Expand Down Expand Up @@ -224,6 +275,12 @@ _.assign(Substitutor, /** @lends Substitutor */ {
return function (match, token) {
var r = substitutor.find(token);

if (r && r.lazy) {
substitutor.lazyVariables.push(r);

return match;
}

r && _.isFunction(r) && (r = r());
r && _.isFunction(r.toString) && (r = r.toString());

Expand Down
79 changes: 79 additions & 0 deletions test/unit/property.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,61 @@ describe('Property', function () {
// resolves all independent unique variables as well as poly-chained {{0}} & {{1}}
expect(Property.replaceSubstitutions(str, variables)).to.eql('{{xyz}}');
});

it('should correctly resolve variables with values as sync fn', function () {
const str = '{{world}}',
variables = new VariableList(null, [
{
key: 'world',
value: () => {
return 'hello';
}
}
]);

expect(Property.replaceSubstitutions(str, variables)).to.eql('hello');
});
});

describe('.replaceSubstitionsLazy', function () {
it('should correctly resolve variables with values as async fn', async function () {
const str = '{{world}}',
variables = new VariableList(null, [
{
key: 'world',
lazy: true,
type: 'function',
value: async () => {
const x = await new Promise((resolve) => {
resolve('hello');
});

return x;
}
}
]);

expect(await Property.replaceSubstitutionsLazy(str, variables)).to.eql('hello');
});

it('should show variables as unresolved with values as async fn with error', async function () {
const str = '{{world}}',
variables = new VariableList(null, [
{
key: 'world',
lazy: true,
type: 'function',
value: async () => {
await new Promise((resolve) => {
resolve('hello');
});
throw new Error('fail');
}
}
]);

expect(await Property.replaceSubstitutionsLazy(str, variables)).to.eql('{{world}}');
});
});

describe('.replaceSubstitutionsIn', function () {
Expand All @@ -442,6 +497,30 @@ describe('Property', function () {
});
});

describe('.replaceSubstitutionsInLazy', function () {
it('should replace with lazy variables', async function () {
const obj = { foo: '{{var}}' },
variables = new VariableList(null, [
{
key: 'var',
type: 'any',
lazy: true,
value: async () => {
const res = await new Promise((resolve) => {
resolve('bar');
});

return res;
}
}
]),
res = await Property.replaceSubstitutionsInLazy(obj, [variables]);

expect(res).to.eql({ foo: 'bar' });
expect(obj).to.eql({ foo: '{{var}}' });
});
});

describe('variable resolution', function () {
it('must resolve variables accurately', function () {
var unresolvedRequest = {
Expand Down
9 changes: 9 additions & 0 deletions test/unit/variable-scope.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,15 @@ describe('VariableScope', function () {
expect(scope.get('var-2')).to.equal('var-2-value');
});

it('should get the specified variable with value as a fn', function () {
var scope = new VariableScope([
{ key: 'var-1', value: () => { return 'var-1-value'; } },
{ key: 'var-2', value: () => { return 'var-2-value'; } }
]);

expect(scope.get('var-2')).to.equal('var-2-value');
});

it('should get last enabled from multi value list', function () {
var scope = new VariableScope([
{ key: 'var-2', value: 'var-2-value' },
Expand Down
20 changes: 20 additions & 0 deletions test/unit/variable.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -473,4 +473,24 @@ describe('Variable', function () {
expect(Variable.isVariable()).to.be.false;
});
});

describe('populate', function () {
it('should populate and replace variable with key', async function () {
const variable = new Variable({
key: 'foo',
value: async () => {
const res = await new Promise((resolve) => {
resolve('bar');
});

return res;
},
type: 'lazy'
});

await variable.populate();

expect(variable.toString()).to.eql('bar');
});
});
});
Loading