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

Add support for chained computed prop dependencies #51

Merged
merged 9 commits into from Jan 17, 2018

Conversation

Projects
None yet
5 participants
@burrows
Copy link
Member

burrows commented Jan 11, 2018

Transis.Model classes have always supported defining props that depend on properties of associated models. So you could do stuff like this:

const Company = Transis.Model.extend('Company', function() {
  this.hasMany('employees', 'Employee');
  this.attr('name', 'string');
});

const Employee = Transis.Model.extend('Employee', function() {
  this.hasOne('company', 'Company');
  this.attr('name', 'string');
  this.attr('salary', 'number');

  this.prop('employerName', {
    cache: true,
    on: ['company.name'],
    get: function(name) { return name; }
  });
});

The Employee#employerName prop works because associated models automatically proxy prop change notifications to the objects they are associated with.

A common mistake with computed props was to write one that had a chained dependency on the result of another computed prop, like this:

const Company = Transis.Model.extend('Company', function() {
  this.hasMany('employees', 'Employee');
  this.attr('name', 'string');

  this.prop('highestPaidEmployee', {
    cache: true,
    on: ['employees', 'employees.salary'],
    get: function(employees) {
      return _.maxBy(employees, 'salary');
    }
  });

  // THIS DID NOT WORK BEFORE
  this.prop('highestPaidEmployeeName', {
    cache: true,
    on: ['hightestPaidEmployee.name'],
    get: function(name) { return name; }
  });
});

Previously the Company#highestPaidEmployeeName prop could not be observed and its cache would not be maintained properly because highestPaidEmployee is not an association and therefore does not proxy its prop changes to the Company object. This PR makes these types of props work as you would expect as long as the dependent computed prop returns a Transis object and has cache: true.

this.__cache__ = this.__cache__ || {};

if (this.__deps__ && this.__deps__[name]) {
if (value && typeof value.proxy === 'function') {

This comment has been minimized.

@burrows

burrows Jan 16, 2018

Member

Unnecessary nesting of the if statements.

@@ -165,11 +176,16 @@ function defineProp(object, name, opts = {}) {
(object.__deps__[prop] = object.__deps__[prop] || []).push(name);

if (prop.indexOf('.') !== -1) {
let segments = prop.split('.'), first = segments[0];
let segments = prop.split('.'), first = segments[0], last = segments[1];

This comment has been minimized.

@BrianMehrman

BrianMehrman Jan 16, 2018

Contributor

is there a reason for assigning first and last before seeing if the segments array is longer than 2?

This comment has been minimized.

@burrows

burrows Jan 16, 2018

Member

Its really just a stylistic preference to initialize variables where they are defined. I suppose we're paying a small performance cost when the length is greater than 2, but thats an error case that would prevent the code from running.

object.notify(k);
object.notify(
// strip '@' suffix if present
k.length > 1 && k[k.length - 1] === '@' && k[k.length - 2] !== '.' ?

This comment has been minimized.

@peterwmwong

peterwmwong Jan 17, 2018

Contributor

Could do .endsWith('@.') to favor clearer code. Not sure about the performance but I don't think it should be too bad... and if it is I could fix that :)

This comment has been minimized.

@burrows

burrows Jan 17, 2018

Member

Here is what the code looks like using endsWith:

k.length > 1 && k.endsWith('@') && !k.endsWith('.@') ?
  k.slice(0, k.length - 1) :
  k

Definitely looks nicer but my performance tests on node v8.4.0 says its 1.7-1.9 times slower.

This comment has been minimized.

@jro619

jro619 Jan 17, 2018

Minor, but if you make another commit I'd suggest making '@' and '.' constants with descriptive names for what they represent, assuming they're used elsewhere in the rest of the implementation.

This comment has been minimized.

@peterwmwong

peterwmwong Jan 17, 2018

Contributor

@burrows Thanks I actually misread the code, I didn't see you were testing for /[^\.]@$/ not /.@$/. I'm guessing you tried regex to?

Sidenote: I'm actually surprised it's not slower :) It's currently implemented in CPP and not CSA (most of my CPP -> CSA has yielded >2x gains in V8)

Sidenote #2: Keep in mind the performance profile of V8 in Node 8 and in the current Chrome is very different.

This comment has been minimized.

@burrows

burrows Jan 17, 2018

Member

Yep, tried regex too. Here is the code:

const strs = ["foobars@", "foobars", "foobars.@", "@"];

const n = 1000000;

let t1;

console.log('testing regex:');
let results1 = [];
t1 = new Date;
for (var i = 0; i < n; i++) {
  for (var j = 0, m = strs.length; j < m; j++) {
    var str = strs[j];

    results1.push(str.replace(/([^.])@$/, '$1'));
  }
}
console.log('regex approach took:', (new Date) - t1);

console.log('testing cached regex:');
let results2 = [];
t1 = new Date;
let re = /([^.])@$/;
for (var i = 0; i < n; i++) {
  for (var j = 0, m = strs.length; j < m; j++) {
    var str = strs[j];

    results2.push(str.replace(re, '$1'));
  }
}
console.log('cached regex approach took:', (new Date) - t1);

console.log('testing index:');
let results3 = [];
t1 = new Date;
for (var i = 0; i < n; i++) {
  for (var j = 0, m = strs.length; j < m; j++) {
    var str = strs[j];

    results3.push(str.length > 1 && str[str.length - 1] === '@' && str[str.length - 2] !== '.' ? str.slice(0, str.length - 1) : str);
  }
}
console.log('index approach took:', (new Date) - t1);

console.log('testing endsWith:');
let results4 = [];
t1 = new Date;
for (var i = 0; i < n; i++) {
  for (var j = 0, m = strs.length; j < m; j++) {
    var str = strs[j];

    results4.push(str.length > 1 && str.endsWith('@') && !str.endsWith('.@') ? str.slice(0, str.length - 1) : str);
  }
}
console.log('endsWith approach took:', (new Date) - t1);

And the results:

testing regex:
regex approach took: 1117
testing cached regex:
cached regex approach took: 1086
testing index:
index approach took: 302
testing endsWith:
endsWith approach took: 538

This comment has been minimized.

@burrows

burrows Jan 17, 2018

Member

Well, just tried this in chrome and firefox and endsWith is actually a little faster in both browsers.

This comment has been minimized.

@burrows

burrows Jan 17, 2018

Member

Updated to use endsWith in 870805a.

this.__proxy__.to.didChange(this.__proxy__.name);
if (this.__proxies__) {
for (let k in this.__proxies__) {
let {object, name} = this.__proxies__[k];

This comment has been minimized.

@peterwmwong

peterwmwong Jan 17, 2018

Contributor

Both of these could be const instead of let.

This comment has been minimized.

@burrows

burrows Jan 17, 2018

Member

Thanks, updated in 886d0fc.

@jro619

jro619 approved these changes Jan 17, 2018

if (this.__proxy__) { this.unproxy(); }

this.__proxy__ = {to, name};
TransisObject.prototype.proxy.call(this, to, name);

This comment has been minimized.

@congwenma

congwenma Jan 17, 2018

Contributor

why not just call this.proxy(to, name)

This comment has been minimized.

@burrows

burrows Jan 17, 2018

Member

This is invoking the super method.

This comment has been minimized.

@congwenma

congwenma Jan 18, 2018

Contributor

wouldn't the lookup invoke that method anyway?

This comment has been minimized.

@burrows

burrows Jan 18, 2018

Member

We have to invoke the proxy method from Transis.Object. Here this.proxy is the method we're in so doing that would cause unbounded recursion and blow the stack.

This comment has been minimized.

@congwenma

congwenma Jan 22, 2018

Contributor

ah, missed that.

@burrows burrows merged commit 4fe7e47 into master Jan 17, 2018

@burrows burrows referenced this pull request Jan 31, 2018

Closed

added TransisObject#proxy #45

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