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

async getters discussed? #15

Closed
jayphelps opened this Issue Apr 23, 2014 · 19 comments

Comments

Projects
None yet
6 participants
@jayphelps
Copy link
Contributor

jayphelps commented Apr 23, 2014

Curious if there's been any discussion about async getters?

AsyncMethod :
    get async PropertyName (StrictFormalParameters)  { FunctionBody }

I can see arguments both ways, but the spec currently allows you to await regular properties, so why not properties that are computed via a getter?

Just looking to start an open dialog if one hasn't already.

Some examples for reference...

Assuming this setup:

function fakeAjax() {
  return new Promise(resolve => {
    setTimeout(() => {
      resolve([new Comment(1, 'First comment')]);
    }, 2000);
  }); 
}

class Post {
  constructor(id, body) {
    this.id = id;
    this.body = body;
  }
}

class Comment extends Post {}

Currently supported by traceur:

class Article extends Post {
  constructor(id, body) {
    super(id, body);
    this._comments = null;
  }

  async fetchComments() {
    if (this._comments === null) {
      this._comments = await fakeAjax();
      this._comments[0].body += ' of the day';
    }

    return this._comments;
  }
}

async function main() {
  let article = new Article(123, 'Hello World');

  // Comments may or may not be loaded yet
  let comments = await article.fetchComments();

  console.assert(comments[0].body === 'First comment of the day');
}

Not currently supported by traceur or spec:

class Article extends Post {
  constructor(id, body) {
    super(id, body);
    this._comments = null;
  }

  get async comments() {
    if (this._comments === null) {
      this._comments = await fakeAjax();
      this._comments[0].body += ' of the day';
    }

    return this._comments;
  }
}

async function main() {
  let article = new Article(123, 'Hello World');

  // Comments may or may not be loaded yet
  let comments = await article.comments;

  console.assert(comments[0].body === 'First comment of the day');
}

But you can work around this spec omission by doing this:

class Article extends Post {
  constructor(id, body) {
    super(id, body);
    this._comments = null;
  }

  async _fetchComments() {
    this._comments = await fakeAjax();
    this._comments[0].body += ' of the day';
  }

  get comments() {
    if (this._comments === null) {
      this._fetchComments();
    }

    return this._comments;
  }
}

async function main() {
  let article = new Article(123, 'Hello World');

  // Comments may or may not be loaded yet
  let comments = await article.comments;

  console.assert(comments[0].body === 'First comment of the day');
}
@domenic

This comment has been minimized.

Copy link
Member

domenic commented Apr 23, 2014

I guess the reason this is a tough call is highlighted by your post. Namely, it doesn't seem appropriate to cache the computation---getter function code is currently executed every time the property is gotten, and async getters should not break this expectation. But then you're doing a potentially-expensive async transform every time.

One use case I could imagine is adding a .loaded getter. E.g.

class Thinger {
  constructor() {
    this._value = getValue(); // returns a promise for an X
  },

  async get loaded {
    await this._value;
    // don't return anything; loaded getters should not have a value
  }
}

But this is probably just better written as

class Thinger {
  constructor() {
    this._value = getValue(); // returns a promise for an X
  },

  get loaded {
    return this._value.then(() => {});
  }
}
@jayphelps

This comment has been minimized.

Copy link
Contributor Author

jayphelps commented Apr 23, 2014

...it doesn't seem appropriate to cache the computation---getter function code is currently executed every time the property is gotten, and async getters should not break this expectation.

@domenic So I can follow, could elaborate on why caching a getter's result internally is a bad practice? IMO, since getters are abstractions so consumers don't need to know the property is even computed, I would think caching the result could even be a best practice if the computation is intensive and a property is preferred over a more revealing method like computeValue().

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Apr 23, 2014

Caching is fine, if it's performed by the getter. But the code that appears inside get x() { ... /* this code here */ } should always run whenever foo.x is gotten.

@jayphelps

This comment has been minimized.

Copy link
Contributor Author

jayphelps commented Apr 23, 2014

@domenic Hmmm. I understand, but I don't know of any case where it would be possible to change that behavior? get async x() { ... /* this code here */ } would always run, would it not? Sorry--I'm a little slow this morning. 👯

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Apr 23, 2014

Well, maybe I made a wrong assumption, but I assumed that you were proposing that your "Not currently supported by traceur or spec" example desugar to your "But you can work around this spec omission by doing this" example.

@jayphelps

This comment has been minimized.

Copy link
Contributor Author

jayphelps commented Apr 23, 2014

@domenic That was a correct assumption, effectively. Thinking about the translation in my head, the promise returned might be different in the former, but would resolve effectively the same. But just to clarify, the caching part is purely a usage example, not that the language itself should cache anything behind the scenes--definitely not.

Maybe this will help clear things up. Here's another example of the proposed, where the developer decided not to ever cache anything:

class Article extends Post {
  get async comments() {
    let comments = await fakeAjax();
    comments[0].body += ' of the day';
    return comments;
  }
}

async function main() {
  let article = new Article(123, 'Hello World');

  // Wait for comments to come back from the server
  let comments = await article.comments;

  console.assert(comments[0].body === 'First comment of the day');
}
@jayphelps

This comment has been minimized.

Copy link
Contributor Author

jayphelps commented Apr 23, 2014

As a real-world example, nested models in Ember Data are now always resolved asynchronously. With the Proxy spec, eventually Ember will be able to get away from using obj.get/set and this syntax would be the natural way to consume nested models.

var article = this.store.find('article', 123);
article.then(function (article) {
  article.get('comments').then(function (comments) {
    console.assert(comments.objectAt(0).get('body') === 'First comment of the day');
  });
});

vs. syntax in 2023

let article = await this.store.find('article', 123);
let comments = await article.comments;
console.assert(comments[0].body === 'First comment of the day');
@lukehoban

This comment has been minimized.

Copy link
Contributor

lukehoban commented Apr 23, 2014

Good topic.

There is no technical reason for this to be disallowed - it is indeed an artificial restriction in the current proposal.

If it was supported though, I don't think it would mean exactly what the proposed rewrite/workaround above suggests. In your code example, the comments accessor property returns either an Array<Comment> or null. What an async accessor property would actually return in this case would be a Promise<Array<Comment>>.

That would still allow the kind of code you show in your Ember Data example, but would require any data-binding framework that saw this kind of object to be able to deal with data-binding promise-valued prototype properties.

The rewrite/workaround that would give you the same behaviour would be:

class Article extends Post {
  constructor(id, body) {
    super(id, body);
    this._comments = null;
  }

  async _fetchComments() {
    if(this._comments === null) {
      this._comments = await fakeAjax();
      this._comments[0].body += ' of the day';
    }
    return this._comments;
  }

  get comments() {
    return this._fetchComments();
  }
}

async function main() {
  let article = new Article(123, 'Hello World');

  // Comments may or may not be loaded yet
  let comments = await article.comments;

  console.assert(comments[0].body === 'First comment of the day');
}

The questions then are:

  1. Is it a generally good idea to have accessor proeprties which are promise-valued?
  2. Assuming it's not (though I'm not even sure that's true), should the language actively disallow this?

I know C# has this same restriction for their async/await feature, though I'm unclear what the exact rationale for the limitation there is.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Apr 23, 2014

My point about .loaded was meant to give an example of when a promise-valued accessor property makes sense.

@jayphelps

This comment has been minimized.

Copy link
Contributor Author

jayphelps commented Apr 23, 2014

@lukehoban You're absolutely right about my workaround vs. yours. I was trying to accomplish that, but mistakenly believed that this._comments = await fakeAjax(); synchronously assigned a Promise that would have been ready on the caller's stack. Nice catch!

The questions then are:

  1. Is it a generally good idea to have accessor properties which are promise-valued?

@domenic's case as well as existing paradigms in the wild, like Ember Data, suggest that this point may be a interesting question to debate but fruitless in the end, much the same as the never-ending debate about whether getters are bad, etc. i.e. there is a clear usage for them as they are being used today.

  1. Assuming it's not (though I'm not even sure that's true), should the language actively disallow this?

I think if you can await obj.prop in general, as you can now per spec, actively disallowing it would be for no benefit but consistency with C# (null reason really) cause likely any JIT deop would apply to needing to arbitrarily await any property, unknown at compile-time whether it may or may not resolve to a Promise. That said, I don't maintain a JS VM so don't have all the facts, but I did stay at a Holiday Inn Express last night.

All of this is IMO, of course. 😄

@jayphelps

This comment has been minimized.

Copy link
Contributor Author

jayphelps commented Apr 23, 2014

@lukehoban @domenic

Here's what I can dig up so far about C#:

Where can’t I use “await”?

Inside of a property getter or setter. Properties are meant to return to the caller quickly, and thus are not expected to need asynchrony, which is geared for potentially long-running operations. If you must use asynchrony in your properties, you can do so by implementing an async method which is then used from your property.

http://blogs.msdn.com/b/pfxteam/archive/2012/04/12/async-await-faq.aspx (sorry they don't have element id's I can directly link to)

This is (probably) not the language designers' reasons, however.

@jayphelps

This comment has been minimized.

Copy link
Contributor Author

jayphelps commented Apr 23, 2014

Looks like @stephentoub wrote that article, maybe we can yank him in here for insight?

@lukehoban

This comment has been minimized.

Copy link
Contributor

lukehoban commented Apr 23, 2014

Actually was just chatting with @stephentoub and Lucian Wischik about this.

They pointed out a key problem.

The desugaring I shared actually has a bug - it will allow the ajax request to be kicked off multiple times, and will only start caching once one of the requests completes. This is almost certainly not the intended behaviour.

The intended behaviour would more likely have been:

class Article extends Post {
  constructor(id, body) {
    super(id, body);
    this._commentsP = null;
  }

  async _fetchComments() {
    var comments = await fakeAjax();
    comments[0].body += ' of the day';
    return comments;
  }

  get comments() {
    if(this._commentsP === null) {
      this._commentsP = this._fetchComments();  
    }
    return this._commentsP;
  }
}

async function main() {
  let article = new Article(123, 'Hello World');

  // Comments may or may not be loaded yet
  let comments = await article.comments;

  console.assert(comments[0].body === 'First comment of the day');
}

However, you can't desugar an async getter into this, because you would be "inside" the promise, and couldn't reach out to cache the promise value you were computing. So, in practice, it wouldn't be possible to write a correctly caching getter without basically writing it in the workaround style.

I think this is basically the ultimate reason the feature doesn't make sense.

@jayphelps

This comment has been minimized.

Copy link
Contributor Author

jayphelps commented Apr 24, 2014

@lukehoban Ohhhh... _fetchComments() actually returns a promise, which resolves to yet another promise, inside which won't assign this._comments until fakeAjax() promise resolves. Similar issue to my original workaround.

Mind fuck

EDIT: DUH JAY.....ignore everything below....it's pointless cause it doesn't use await inside the getter so async isn't necessary at all. Sorry for the brain fart.

That said...I'm very stubborn. What about this?

class Article extends Post {
  constructor(id, body) {
    super(id, body);
    this._comments = null;
  }

  get async comments() {
    if (this._comments === null) {
      this._comments = fakeAjax();
      this._comments.then((comments) => {  
        comments[0].body += ' of the day';
      });
    }

    return this._comments;
  }
}

To test this, I instead used a method..which, if I understand the issue correctly, should suffer from the same caching inside a promise issue:

let ajaxCallCount = 0;

function fakeAjax() {
  ajaxCallCount++;

  return new Promise(resolve => {
    setTimeout(() => {
      resolve([new Comment(1, 'First comment')]);
    }, 2000);
  }); 
}

class Post {
  constructor(id, body) {
    this.id = id;
    this.body = body;
  }
}

class Comment extends Post {}

class Article extends Post {
  constructor(id, body) {
    super(id, body);
    this._comments = null;
  }

  async getComments() {
    if (this._comments === null) {
      this._comments = fakeAjax();
      this._comments.then((comments) => {  
        comments[0].body += ' of the day';
      });
    }

    return this._comments;
  }
}

async function test1() {
  let article = new Article(123, 'Hello World');

  // First time calling, promise is created and fakeAjax sent
  let comments = await article.getComments();

  assertCommon('test1', comments, 1);
}

async function test2() {
  let article = new Article(123, 'Hello World');

  // First time awaiting, promise is created and fakeAjax sent
  let comments = await article.getComments();

  // Second time, 
  let commentsAgain = await article.getComments();

  assertCommon('test2', comments, 1);
}

async function test3() {
  let article = new Article(123, 'Hello World');

  // First time, let's NOT await the result
  console.assert(article.getComments() instanceof Promise);
  // Second time called should returned cached promise
  console.assert(article.getComments() instanceof Promise);

  // Third time we await
  let comments = await article.getComments();

  assertCommon('test3', comments, 1);
}

function assertCommon(testName, comments, expectedAjaxCallCount) {
  console.assert(comments[0].body === 'First comment of the day', testName + ': comment body should have been correctly altered.');
  console.assert(ajaxCallCount === expectedAjaxCallCount, testName + ': fakeAjax should only be called ' + expectedAjaxCallCount + ' times. Was called: ' + ajaxCallCount);
  console.log('PASSED TEST: ' + testName);
}

console.log('------------------------', (new Date).toLocaleTimeString());
test1().then(() => {
  ajaxCallCount = 0;

  test2().then(() => {
    ajaxCallCount = 0;

      test3();
  });
});
@jayphelps

This comment has been minimized.

Copy link
Contributor Author

jayphelps commented Apr 24, 2014

Let's try that again...This appears to pass my tests. Replacing get async comments() with async getComments() to test.

class Article extends Post {
  constructor(id, body) {
    super(id, body);
    this._comments = null;
  }

  get async comments() {
    if (this._comments === null) {
      this._comments = fakeAjax();
      let comments = await this._comments;
      comments[0].body += ' of the day';
    }

    return this._comments;
  }
}

By all means, find the flaw.

I think this could also be written as:

  get async comments() {
    if (this._comments === null) {
      await (this._comments = fakeAjax());
      this._comments[0].body += ' of the day';
    }

    return this._comments;
  }
@zenparsing

This comment has been minimized.

Copy link
Member

zenparsing commented Apr 24, 2014

Agree with Luke - let's keep things simple and rock-solid.

Also, at a more conceptual level, async functions always express operations. Accessors, on the other hand, express properties. Those properties are virtual, of course, and accessing them might trigger off asynchronous operations, but to the consumer it must appear as a property of the object.

@arv

This comment has been minimized.

Copy link
Member

arv commented May 19, 2014

Good topic. I agree with Kevin, getters are supposed to represent properties which represents state. Therefore these are better done without async even if the the getter returns a Promise.

Lets leave async getters out of this proposal.

@jayphelps

This comment has been minimized.

Copy link
Contributor Author

jayphelps commented May 19, 2014

👍

@spion

This comment has been minimized.

Copy link

spion commented Mar 6, 2017

I'm not sure if this restriction will continue to make sense. Decorators in particular make it possible to move concerns such as caching out of the getter code, so I think its worthwhile to revisit this for the future?

jtguggedal referenced this issue in jtguggedal/thingy_web_bluetooth Dec 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.