Remove Promise support from core, add async() directive. #555

justinfagnani · 2018-10-10T03:38:29Z

Removes built-in Promise support. Adds an async() directive as a successor to until() that is required to render Promises. async() handles race conditions, works with attribute, boolean attribute, property and event bindings. It also works as a nested directive value with other directives like repeat().

While this only removes a small amount of current core library code, more core code would have be required to handle attributes, etc., and the race conditions, mainly because current Part factoring has us sharing very little code between Part implementations. It’s possible that we could refactor Parts in the future to share more value-handling code and bring back built-in Promise support in the future. Adding that would not be a breaking change, but we need to remove support before 1.0 since removing later would be a breaking change.

This change keeps directives/until.js to re-export async to ease the transition. It’s deprecated and will be removed before 1.0.

Fixes #550
Closes #272, #544, #451 and #458

src/directives/async.ts

bgotink · 2018-10-10T19:50:17Z

src/directives/async.ts

+          return;
+        }
+        promisedValue.value = value;
+        part.setValue(value);


What if the value of the part has changed to a non-async value by now? then callback checks whether a new async(promise) was passed but it doesn't check for non-async(promise) value

This isn't currently supported. There's really no way to tell that this directive has been removed - it has to be used somewhat statically.

I though we could solve it by adding a new property to part, readonly originalValue: any, which would represent the value that gets set from the template regardless of whether it's a directive or something else entirely. That would allow the async directive to check whether it's still the active value in the case of a template like

html`${async(somePromise, 'temp')}`

but it would only work if until is used directly inside of the part, not in cases where it's wrapped in another directive, e.g.

html`${ when(value instanceof Promise, () => async(value, 'temp'), () => value) }`

I think this is something to be designed and solved for directives in general, not just async. Right now there are lots of sharp edges when directives aren't essentially "static" and top-level. Replacing directives can lead to other issues too.

bgotink · 2018-10-11T18:45:00Z

src/directives/async.ts

+interface PromisedValue<T = unknown> {
+  promise: Promise<T>;
+  resolved: boolean;
+  value?: T;


This property is unused (it's set at line 55 but never read)
Did you mean to add

if (promisedValue.resolved) { part.setValue(promisedValue.value); }

in the if-check at lines 33-41?

ah, turns out it's just not needed

LarsDenBakker · 2018-10-11T20:16:40Z

src/directives/async.ts

+        part.setValue(defaultContent);
+      }
+
+      Promise.resolve(promise).then((value: any) => {


Why is an extra Promise.resolve needed? Is that to support non-promise values?

Yeah, and mostly this is safer than blindly calling .then()

In Canary I have queueMicrotask which does the same. Maybe it makes sense to call that when available, as it might have slightly less overhead

https://www.chromestatus.com/features/5111086432911360

justinfagnani · 2018-10-16T18:22:59Z

@sorvell @kevinpschaaf PTAL

kevinpschaaf

Minor suggestion for code reduction/readability, otherwise LGTM.

kevinpschaaf · 2018-10-19T23:23:52Z

src/directives/async.ts

+      // The following code only ever runs once per Promise instance
+      promisedValue = {promise, resolved: false};
+      promises.set(part, promisedValue);
+      if (defaultContent !== noChange) {


This factoring is slightly more readable to me and avoids duplicating the part.setValue(defaultContent) code (sorry for rewriting, it was easier than explaining the change):

export const async = directive( (promise: Promise<any>, defaultContent: unknown = noChange) => (part: Part) => { let promisedValue = promises.get(part); // If the promise changed, record the new promise and await it if (promisedValue === undefined || promisedValue.promise !== promise) { promisedValue = {promise, resolved: false}; promises.set(part, promisedValue); Promise.resolve(promise).then((value: unknown) => { const currentPromisedValue = promises.get(part); promisedValue.resolved = true; if (currentPromisedValue === promisedValue) { part.setValue(value); part.commit(); } }); } // If the promise has not yet resolved, set/update the defaultContent if (!promisedValue.resolved && defaultContent !== noChange) { part.setValue(defaultContent); } });

Used your code. Thanks :)

kevinpschaaf · 2018-10-19T23:31:46Z

src/directives/async.ts

+        if (currentPromisedValue !== promisedValue) {
+          return;
+        }
+        part.setValue(value);


Gah, also looks like we're forgetting to set promisedValue.resolved = true, right? Should add a test that catches this case? Or did I miss something?

Added test that fails without setting resolved

justinfagnani

@kevinpschaaf PTAL

justinfagnani · 2018-10-31T20:27:36Z

src/directives/async.ts

+      // The following code only ever runs once per Promise instance
+      promisedValue = {promise, resolved: false};
+      promises.set(part, promisedValue);
+      if (defaultContent !== noChange) {


Used your code. Thanks :)

justinfagnani · 2018-10-31T20:27:43Z

src/directives/async.ts

+        if (currentPromisedValue !== promisedValue) {
+          return;
+        }
+        part.setValue(value);


Added test that fails without setting resolved

justinfagnani · 2018-10-31T20:41:20Z

@kevinpschaaf the code as currently intended to work should be good now, but I'm not 100% sure on the until-style API.

One API share we've talked about is accepting a varargs list of Promises:

html`${async(result1, result2, fallback)}`

Where result1 is prefered over result2 which is prefered over fallback. Any value could be sync or async. The defaultContent parameter would no longer be special, just assumed to be sync or a faster-resolving Promise.

Another potential change is that I think we might want to also support passing a function and invoking it. This will make using async functions easier, since they won't have to be immediate invoked:

<div>
  html`${async(async () => {
    const result = await fetchSomething();
    return html`<h1>${result.title}</h1>`;
  })}`
</div>

The problem there is the unfortunate async(async run. This makes me question the name...

It also kind of makes me want to keep text-position Promise support in core, and even expand text-position types to invoke functions, since they don't render to anything useful anyway. Then we support this:

<div>
  html`${async () => {
    const result = await fetchSomething();
    return html`<h1>${result.title}</h1>`;
  }}`
</div>

But I'm still apprehensive about the usability danger of Promises being supported in text-position but not attribute position. That could be added later too...

justinfagnani · 2018-11-21T05:20:17Z

Coming back around to this. My previous comment had a few problems, mainly that we don't want to support invoking an async function each render. That would produce a new Promise each render and possibly re-render the placeholders while it resolves. The runAsync() directive generator pattern is the safe way to do that. This means the async name is fine.

I do think it's better to accept varargs of Promise or sync values and have them cascade in priority from left to right. I'll see if that's diable in a simple implementation.

I'll update, rebase and ping when done.

justinfagnani · 2018-11-25T00:43:07Z

@kevinpschaaf PTAL

I got the generalized version working so that async() accepts any number of Promises and non-Promises and renders them in priority order. Handling cases where the arguments change over time is not incredibly obvious, but wasn't that bad either, so I added that with hopefully enough comments to explain and test coverage for it.

kevinpschaaf

LGTM after adding test for rendering unresolved promises twice while swapping positions.

Removes built-in Promise support. Adds an async() directive as a successor to until() that is requires to render Promises. async() handles race conditions, works with attribute, boolean attribute, property and event bindings. It also works as a nested directive value with other directives like repeat(). While this only removes a small amount of current core library code, more core code would have be required to handle attributes, etc., and the race conditions, mainly because current Part factoring has us sharing very little code between Part implementations. It’s possible that we could refactor Parts in the future to share more value-handling code and bring back built-in Promise support in the future. Adding that would not be a breaking change, but we need to remove support before 1.0 since removing later would be a breaking change. This change keeps directives/until.js to re-export async to ease the transition. It’s deprecated and will be removed before 1.0.

Removes built-in Promise support. Requires the until() directive to handle Promises. until() is updated to handle race conditions, work with attributes, boolean attributes, properties and event bindings. While this only removes a small amount of current core library code, more core code would have be required to handle attributes, etc., and the race conditions, mainly because current Part factoring has us sharing very little code between Part implementations. It’s possible that we could refactor Parts in the future to share more value-handling code and bring back built-in Promise support in the future. Adding that would not be a breaking change, but we need to remove support before 1.0 since removing later would be a breaking change.

justinfagnani requested a review from kevinpschaaf October 10, 2018 03:38

justinfagnani force-pushed the async-directive branch from d1871ae to 88a967c Compare October 10, 2018 03:41

43081j mentioned this pull request Oct 10, 2018

Consider removing built-in Promise support in favor of until() directive. #550

Closed

bgotink reviewed Oct 10, 2018

View reviewed changes

src/directives/async.ts Outdated Show resolved Hide resolved

bgotink reviewed Oct 10, 2018

View reviewed changes

src/directives/async.ts Outdated Show resolved Hide resolved

bgotink reviewed Oct 10, 2018

View reviewed changes

bgotink reviewed Oct 11, 2018

View reviewed changes

LarsDenBakker reviewed Oct 11, 2018

View reviewed changes

justinfagnani force-pushed the async-directive branch from d22863e to 4369e05 Compare October 16, 2018 18:07

justinfagnani requested a review from sorvell October 16, 2018 18:23

kevinpschaaf requested changes Oct 19, 2018

View reviewed changes

kevinpschaaf reviewed Oct 19, 2018

View reviewed changes

andria-dev approved these changes Oct 29, 2018

View reviewed changes

csvn mentioned this pull request Oct 29, 2018

Fixed directive examples bad code #596

Closed

justinfagnani commented Oct 31, 2018

View reviewed changes

justinfagnani force-pushed the async-directive branch from ac8f1ac to 2fd1c3e Compare October 31, 2018 21:11

mariusGundersen mentioned this pull request Nov 18, 2018

Make it possible to extend the values that a NodePart can render #638

Closed

justinfagnani force-pushed the async-directive branch from 2fd1c3e to 80cedcf Compare November 24, 2018 02:46

justinfagnani changed the title ~~Move Promise support to directives.~~ Remove Promise support from core, add async() directive. Nov 25, 2018

justinfagnani mentioned this pull request Nov 28, 2018

Promised attributes are not rendered properly #272

Closed

kevinpschaaf approved these changes Nov 28, 2018

View reviewed changes

justinfagnani added 5 commits November 28, 2018 15:26

Add test with nested async directives

c07cb7c

Address review comments

7a015f0

Address feedback and format

b07ab67

Address review comments

05d68a4

justinfagnani added 6 commits November 28, 2018 15:26

checkpoint

45a3366

Update async() to take varargs.

4e6efcb

format

50693d7

Update changeling and docs

a4730e4

Add comments. Fix rendering of changing arguments.

748602b

Add another test

6e5868f

justinfagnani force-pushed the async-directive branch 3 times, most recently from f5858f7 to d8bbf9e Compare November 28, 2018 23:48

Rename async() to until()

86e4768

justinfagnani force-pushed the async-directive branch from d8bbf9e to 86e4768 Compare November 29, 2018 00:06

justinfagnani merged commit a219606 into master Nov 29, 2018

This was referenced Nov 29, 2018

Asynchronous method in a conditional attribute notation #544

Closed

_commitPromise is inefficient #451

Closed

Improve promise performance #458

Closed

dorivaught added this to the 1.0.0 milestone Dec 3, 2018

justinfagnani deleted the async-directive branch February 9, 2019 23:52

terrorizer1980 mentioned this pull request Sep 9, 2024

[Snyk] Upgrade: lit-html, approximate-number, twitter-text terrorizer1980/twitter-user#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Promise support from core, add async() directive. #555

Remove Promise support from core, add async() directive. #555

justinfagnani commented Oct 10, 2018 •

edited by kevinpschaaf

Loading

bgotink Oct 10, 2018 •

edited

Loading

justinfagnani Oct 10, 2018

bgotink Oct 11, 2018 •

edited

Loading

justinfagnani Oct 16, 2018

bgotink Oct 11, 2018

justinfagnani Oct 16, 2018

LarsDenBakker Oct 11, 2018

justinfagnani Oct 16, 2018

kenchris Oct 16, 2018

justinfagnani commented Oct 16, 2018

kevinpschaaf left a comment

kevinpschaaf Oct 19, 2018 •

edited

Loading

justinfagnani Oct 31, 2018

kevinpschaaf Oct 19, 2018

justinfagnani Oct 31, 2018

justinfagnani left a comment

justinfagnani Oct 31, 2018

justinfagnani Oct 31, 2018

justinfagnani commented Oct 31, 2018

justinfagnani commented Nov 21, 2018

justinfagnani commented Nov 25, 2018

kevinpschaaf left a comment

Remove Promise support from core, add async() directive. #555

Remove Promise support from core, add async() directive. #555

Conversation

justinfagnani commented Oct 10, 2018 • edited by kevinpschaaf Loading

bgotink Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgotink Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinfagnani commented Oct 16, 2018

kevinpschaaf left a comment

Choose a reason for hiding this comment

kevinpschaaf Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinfagnani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinfagnani commented Oct 31, 2018

justinfagnani commented Nov 21, 2018

justinfagnani commented Nov 25, 2018

kevinpschaaf left a comment

Choose a reason for hiding this comment

justinfagnani commented Oct 10, 2018 •

edited by kevinpschaaf

Loading

bgotink Oct 10, 2018 •

edited

Loading

bgotink Oct 11, 2018 •

edited

Loading

kevinpschaaf Oct 19, 2018 •

edited

Loading