Promise support in liquidMethodMissing & expressions #285
Conversation
almousa1990
changed the title
| @@ -3,7 +3,7 @@ export abstract class Drop { | |||
| return undefined | |||
| } | |||
|
|
|||
| public liquidMethodMissing (key: string): Promise<string | undefined> | string | undefined { | |||
| public liquidMethodMissing (key: string): Promise<any | undefined> | any | undefined { | |||
There was a problem hiding this comment.
After changed to any, maybe | undefined is no longer nessessary?
| const scope = { 'var': Promise.resolve('var') } | ||
| const html = await liquid.parseAndRender(src, scope) | ||
| return expect(html).to.equal('success') | ||
| }) |
There was a problem hiding this comment.
It would be even better if you also add a case demonstrating {{ var }} also works.
| if (isNil(obj)) return obj | ||
| obj = toLiquid(obj) | ||
| if (obj instanceof Promise) { | ||
| return obj.then(resolvedObj => readProperty(resolvedObj, key)) |
There was a problem hiding this comment.
I think we need await the obj when calling readProperty rather than inside readProperty, it's passed in by the caller after all.
If we also need to support toLiquid returning a Promise, we should make it explicit by obj = yield toLiquid(obj) and a corresponding test case. (I'm not sure if you want this, it's up to you).
There was a problem hiding this comment.
But don't we still need to await inside readProperty if the promise was an object with properties?
There was a problem hiding this comment.
I believe not, readProperty is not recursive and only called from getFromScope. We can yield the obj before pass into the readProperty, like:
readProperty(yield scope, path)
There was a problem hiding this comment.
Since i don't see a way to implement these changes without introducing breaking changes, i am going to close this PR.
Appreciated your time
| @@ -67,9 +67,12 @@ export class Context { | |||
| } | |||
There was a problem hiding this comment.
These lines are possibly broken:
.reduce is executed syncly, and the following case will fail:
const val = await liquid.parseAndRender('{{ foo.bar }}', { foo: Promise.resolve({bar: 'BAR'}) })
expect(var).to.equal('BAR')I suggest replace the .reduce() with for...of and yield readProperty().
There was a problem hiding this comment.
Does it mean that getFromScope has to be generator function? i think this can be a breaking change.\
I tried the above test and it passed.
There was a problem hiding this comment.
It's OK to leave it alone. I'll handle this latter.
adds a promise support in liquidMethodMissing. This should extends Drop capability. Should fix #274
Also, adds a promise support in expressions. Should address #276
This is my first pull request, i ran the tests and all passed.