Add runAsync() directive #657

justinfagnani · 2018-11-25T00:46:34Z

Adds a runAsync directive.

This directive takes a key object, task function, and a set of templates to render based on the state of the task (initial, pending, success and failure).

The API is slightly different than what I referenced in #649: This is a plain directive, and not a factory. A factory can be created by currying this directive, or a directive tied to a specific task function can be created via composition. The templates have also moved to a object with named properties.

Fixes #649

kevinpschaaf · 2018-11-28T23:43:48Z

src/directives/run-async.ts

+            new CustomEvent('pending-state', {
+              composed: true,
+              bubbles: true,
+              detail: {promise: pendingPromise}


Consider trying to reuse the same pendingPromise when a new promise supersedes a previous unresolved one, and avoid firing the event multiple times?

kevinpschaaf · 2018-11-28T23:43:56Z

src/directives/run-async.ts

+
+const runs = new WeakMap<Part, AsyncRunState>();
+
+export type Task<K> = (key: K, options: {signal?: AbortSignal}) =>


As discussed, key (singular) should be changed to dependencies (plural, array)

kevinpschaaf · 2018-11-28T23:53:43Z

src/directives/run-async.ts

+          if (currentRunState !== runState) {
+            return;
+          }
+          part.setValue(success(value));


Ensure the template result for the current success/pending/initial/failure state is re-evaluated (with e.g. the last resolved/error'ed value) for each call to the directive, not just on dependency invalidation, to avoid a footgun where other props in the closure are used in the TR but do not update on change.

We should also be clear in the docs that the dependencies (née key) are what trigger the async function running and thus generating a new promise; however they do not "guard" the template result functions in the way that guard directive would. If the TR functions are expensive, then they would need to nest a guard directive in that value (unless we remove that ability -- in which case there is a slight capability hole there).

kevinpschaaf · 2018-11-29T00:00:23Z

src/test/directives/run-async_test.ts

+    });
+    assert.equal(stripExpressionMarkers(container.innerHTML), 'Pending');
+    await 0;
+    assert.equal(stripExpressionMarkers(container.innerHTML), 'Initial');


Make runAsync wait a microtask after running the async fn to get a shot at rendering initial before pending?

kevinpschaaf · 2018-11-29T00:09:39Z

src/test/directives/run-async_test.ts

+const hasAbortController = typeof AbortController === 'function';
+const testIfHasAbortController = hasAbortController ? test : test.skip;
+
+suite('runAsync', () => {


Add tests:

Multiple dependencies

Add use of props from render closure in template result functions, and ensure they re-evaluate correctly each render

Ensure pending-state event is not fired in initial state

googlebot · 2019-02-09T23:52:07Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Westbrook · 2019-02-10T22:39:36Z

src/directives/run-async.ts

+      rejectPending = rej;
+    });
+    const promise = task(key, {signal: abortSignal});
+    // The state is immediately 'pending', since the function has been


Does this mean that even if the promise can't be run with the data available it will take always take two renders to get to initial? One pass for pending and then a second (assuming throw new InitialStateError()) to trigger initial. If so, that seems confusing to end users.

This may be conflating the responsibilities of lit-html and a component system (i.e. LitElement) that relies on it, but I would expect more mainstream usage of this to rely on a key that powered the promise in someway so an initial call where key === undefinedorkey === ''would provide theinitial` render template in a single render pass.

In this case, would you expect there to be additional wrapping of or an alternative to this directive to achieve that functionality? or, would it make sense to include that by default?

dbatiste · 2019-04-30T14:28:30Z

src/directives/run-async.ts

+  }
+
+  // If the promise has not yet resolved, set/update the defaultContent
+  if ((currentRunState === undefined || currentRunState.state === 'pending') &&


Hi @justinfagnani : I am experimenting with this a bit since you mentioned it last week. I notice that the pending state is not applied on first key/dependency change after a previous success state - it remains in the success state. Another key change is required for the pending state to be applied.

I could be doing something incorrectly, but I think : on line#93 runs.set(part, runState); is called to set a new pending state, but currentRunState is still the previous success state, so pending() is not called here. Once the directive is run again (ex. second key-stroke in the search demo), currentRunState is initialized with the most recent pending state, and then pending() is called here. It is perhaps not super noticeable when there are many changes (like input event). The absent intermediate pending state was fairly obvious in my case where there aren't multiple key changes quickly in succession.

I would agree with this. Won't we need to reacquire the values for the current run if we ever want to be able to return to the "pending" state on subsequent uses?

I've been playing with the code shaped this way at https://stackblitz.com/edit/lit-element-run-async-2?file=run-async.ts

Suggested change

if ((currentRunState === undefined || currentRunState.state === 'pending') &&

const runState = runs.get(part);

if ((runState === undefined || runState.state === 'pending') &&

Westbrook · 2019-07-31T12:37:13Z

src/directives/run-async.ts

+      // Wait a microtask for the initial render of the Part to complete
+      await 0;
+      const currentRunState = runs.get(part);
+      if (currentRunState === runState && currentRunState.state === 'pending') {


If you only dispatch a promise when currentRunState.state === 'pending' an action that immediate throws new InitialStateError() won't be provided the pendingPromise reference to manage the reject that happens on line 108. Should this be updated to:

Suggested change

if (currentRunState === runState && currentRunState.state === 'pending') {

if (currentRunState === runState && (currentRunState.state === 'pending' || currentRunState.state === 'initial')) {

justinfagnani · 2021-03-16T01:33:53Z

Closing in favor of reactive controllers in lit-next.

justinfagnani requested review from sorvell and kevinpschaaf November 26, 2018 01:30

justinfagnani changed the title ~~WIP: Add runAsync() directive~~ Add runAsync() directive Nov 26, 2018

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

kevinpschaaf requested changes Nov 29, 2018

View reviewed changes

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

kevinpschaaf requested changes Nov 29, 2018

View reviewed changes

dorivaught modified the milestones: 1.0.0, 1.x Nov 29, 2018

justinfagnani added 8 commits February 9, 2019 15:47

WIP: no tests yet

54d746f

Add tests

3092c95

Move templates to object with named properties

4ba58e8

Add some TODOs

39ed3f1

Feature detect AbortController

0130f1e

Small comment/type changes

54fa34c

Update changelog

ad2a7a0

Add docs

8ce8cc6

justinfagnani force-pushed the run-async branch from c487b1d to 8ce8cc6 Compare February 9, 2019 23:51

justinfagnani changed the base branch from async-directive to master February 9, 2019 23:52

Westbrook reviewed Feb 10, 2019

View reviewed changes

justinfagnani removed this from the 1.1.0 milestone Feb 11, 2019

dbatiste reviewed Apr 30, 2019

View reviewed changes

dbatiste mentioned this pull request Jun 10, 2019

Option 3 - Fix icon property update BrightspaceUI/core#113

Merged

Westbrook reviewed Jul 31, 2019

View reviewed changes

justinfagnani closed this Mar 16, 2021

justinfagnani deleted the run-async branch March 16, 2021 01:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add runAsync() directive #657

Add runAsync() directive #657

justinfagnani commented Nov 25, 2018 •

edited

kevinpschaaf Nov 28, 2018 •

edited

kevinpschaaf Nov 28, 2018

kevinpschaaf Nov 28, 2018 •

edited

kevinpschaaf Nov 29, 2018

kevinpschaaf Nov 29, 2018

googlebot commented Feb 9, 2019

Westbrook Feb 10, 2019

dbatiste Apr 30, 2019

Westbrook Jul 25, 2019 •

edited

Westbrook Jul 31, 2019

justinfagnani commented Mar 16, 2021


		const runs = new WeakMap<Part, AsyncRunState>();

		export type Task<K> = (key: K, options: {signal?: AbortSignal}) =>

	if ((currentRunState === undefined \|\| currentRunState.state === 'pending') &&
	const runState = runs.get(part);
	if ((runState === undefined \|\| runState.state === 'pending') &&

	if (currentRunState === runState && currentRunState.state === 'pending') {
	if (currentRunState === runState && (currentRunState.state === 'pending' \|\| currentRunState.state === 'initial')) {

Add runAsync() directive #657

Add runAsync() directive #657

Conversation

justinfagnani commented Nov 25, 2018 • edited

kevinpschaaf Nov 28, 2018 • edited

Choose a reason for hiding this comment

kevinpschaaf Nov 28, 2018

Choose a reason for hiding this comment

kevinpschaaf Nov 28, 2018 • edited

Choose a reason for hiding this comment

kevinpschaaf Nov 29, 2018

Choose a reason for hiding this comment

kevinpschaaf Nov 29, 2018

Choose a reason for hiding this comment

googlebot commented Feb 9, 2019

Westbrook Feb 10, 2019

Choose a reason for hiding this comment

dbatiste Apr 30, 2019

Choose a reason for hiding this comment

Westbrook Jul 25, 2019 • edited

Choose a reason for hiding this comment

Westbrook Jul 31, 2019

Choose a reason for hiding this comment

justinfagnani commented Mar 16, 2021

justinfagnani commented Nov 25, 2018 •

edited

kevinpschaaf Nov 28, 2018 •

edited

kevinpschaaf Nov 28, 2018 •

edited

Westbrook Jul 25, 2019 •

edited