feat(context): add more utils to resolve valueOrPromises #1303

raymondfeng · 2018-05-04T17:08:38Z

This PR is spin-off from #983. It extracts a few utilities to help handle ValueOrPromise.

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

shimks · 2018-05-04T18:01:28Z

packages/context/src/resolver.ts

-      return Object.assign(inst, propertiesOrPromise);
+    return new ctor(...args);
+  });
+  return resolveValueOrPromise(propertiesOrPromise, props => {


is there a reason as to why we don't use a promise style here?

We need to do the work synchronously or asynchronously based on the return value. The usage of ValueOrPromise to allow us to call context.getSync() and context.get().

bajtos · 2018-05-09T11:21:51Z

packages/context/src/value-promise.ts

+  const valueOrPromise = resolver(sourceVal);
+  return resolveValueOrPromise(valueOrPromise, v => {
+    if (evaluator(sourceVal, v)) return v;
+    else return resolveUntil(source, resolver, evaluator);


I believe this will cause stack overflow error when all values are resolved synchronously and the Iterator returns enough items to overflow the stack.

I am not sure if there is any elegant way how to keep supporting both sync/async and using resolveValueOrPromise under the hood. I see two ways forward that should be relatively straightforward.

Modify resolveUntil to always work asynchronously - ensure the recursive call of resolveUntil is always deferred to the next tick of event loop or promise micro-queue. This will allow us to use resolveValueOrPromise.

Rework the code to iterate inside the same function, drop resolveValueOrPromise. This will preserve sync/async semantics.

Before you start working on this, please add a test that's triggering stack overflow error - this will verify the assumptions and serve as a proof that the new version works correctly.

bajtos · 2018-05-09T11:24:51Z

packages/context/src/value-promise.ts

+ * @param resolver The resolve function that maps the source value to a result
+ * @param evaluator The evaluate function that decides when to stop
+ */
+export function resolveUntil<T, V>(


I find the name resolveUntil confusing. In my mind, we resolve values from a context (IoC container), but there is no context involved here. I am proposed to rename this method to mapUntil.

I choose resolveUntil to be consistent with other methods such as resolveList.

Fair enough 👍

bajtos · 2018-05-09T11:32:49Z

packages/context/src/value-promise.ts

+ * @param valueOrPromise The value or promise
+ * @param resolver A function that maps the source value to a value or promise
+ */
+export function resolveValueOrPromise<T, V>(


I find the name resolveValueOrPromise confusing. In my mind, we resolve values from a context (IoC container), but there is no context involved here. I am proposed to rename this function to mapValueOrPromise or perhaps transformValueOrPromise.

OTOH, it is true that Promises can be resolved too, so maybe "resolve" is no that bad.

@strongloop/sq-lb-apex Thoughts?

I renamed it to transformValueOrPromise

bajtos · 2018-05-09T11:34:10Z

packages/context/test/unit/value-promise.unit.ts

+    expect(result).to.eql('A');
+  });
+
+  it('handles a rejected promise', () => {


Please add another test to verify how sync errors are handled.

resolveValueOrPromise('a', v => {throw new Error(v);});

raymondfeng · 2018-05-09T15:50:29Z

I believe this will cause stack overflow error when all values are resolved synchronously and the Iterator returns enough items to overflow the stack.

Good catch. I rewrote the implementation to avoid that. A few tests are added to cover the corner case.

raymondfeng · 2018-05-09T15:50:42Z

@bajtos PTAL

bajtos · 2018-05-09T16:02:18Z

packages/context/src/value-promise.ts

+ * @param resolver The resolve function that maps the source value to a result
+ * @param evaluator The evaluate function that decides when to stop
+ */
+export function resolveUntil<T, V>(


Fair enough 👍

bajtos · 2018-05-09T16:02:59Z

packages/context/src/value-promise.ts

+    const valueOrPromise = resolver(sourceVal);
+    if (isPromiseLike(valueOrPromise)) {
+      return valueOrPromise.then(v => {
+        if (evaluator(sourceVal, v)) {


Should we allow evaluator to return a Promise<boolean> too?

We can add it if a use case comes up.

raymondfeng requested a review from bajtos as a code owner May 4, 2018 17:08

shimks reviewed May 4, 2018

View reviewed changes

bajtos suggested changes May 9, 2018

View reviewed changes

raymondfeng force-pushed the resolve-value-or-promise branch from f92f81b to dd119f8 Compare May 9, 2018 15:46

feat(context): add more utils to resolve valueOrPromises

b07954c

raymondfeng force-pushed the resolve-value-or-promise branch from dd119f8 to b07954c Compare May 9, 2018 16:04

bajtos approved these changes May 9, 2018

View reviewed changes

raymondfeng merged commit cc55ef5 into master May 9, 2018

raymondfeng deleted the resolve-value-or-promise branch May 9, 2018 16:12

feat(context): add more utils to resolve valueOrPromises #1303

feat(context): add more utils to resolve valueOrPromises #1303

Uh oh!

Conversation

raymondfeng commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented May 9, 2018

Uh oh!

raymondfeng commented May 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

raymondfeng commented May 4, 2018 •

edited

Loading