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.

Sign up for GitHub

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

Jump to bottom

Promise support in scope.get #65

Closed
cmawhorter opened this issue Mar 20, 2018 · 6 comments
Closed

Promise support in scope.get #65

cmawhorter opened this issue Mar 20, 2018 · 6 comments
Labels
breaking change feature requested
Milestone
8.0.0

Comments

@cmawhorter
Copy link
Contributor

The ruby implementation of liquid supports async/lazy data access so that data can be loaded on-demand.

This is described here in the rust version of of liquid: cobalt-org/liquid-rust#118 (comment)

scope.get should work with promises so that I could do {{ some_async_product.price }} and have the product data load via API call that returns a promise.

This would be a big, breaking change, but I've implemented it in our fork and it was definitely not easy, but at this point it is working well. (I do worry about race conditions with scope.push/pop, though it hasn't been a problem yet.)
@harttle
Copy link
Owner

harttle commented Mar 21, 2018

If getFromContext is called only when the corresponding block is rendering, race condition can be avoided since, typically, scope.push is called before the block rendering, and scope.pop is called after the block has been rendered.
It's great to have promise support in scope.get. We can open a branch to do that.

@harttle harttle mentioned this issue Mar 21, 2018
Closed
@cmawhorter
Copy link
Contributor Author

submitting a PR here is going to be difficult. in my fork i've converted to es6, changed project structure on disk, and changed the eslint/code style requirements and made many, many backwards incompat changes that brings liquidjs more in line with shopify/liquid, but definitely breaks backwards compat.

i'd love to get these changes merged here, but i'm not sure about how best to proceed. none of them are compartmentalized, so i can submit a PR, but it'd probably be an all-or-nothing situation.

scope.push is called before the block rendering

in my fork after implementing promises, i think it might be possible to have multiple blocks in a partial rendering state at the same time. like i said, i've never experienced this, but it feels like it is possible.

@cmawhorter
Copy link
Contributor Author

FYI - i plan to refactor into async/await once IE Edge ships chromium. that might be a good time to figure out how to merge, if you're interested.

@harttle
Copy link
Owner

harttle commented Dec 13, 2018

That would be great! We've shipped to ES6 and introduced babelify since v5.3.0-0 to generate a fully compatible ES5 version for #83 . I can help if there're any conflict with the recent features.

@harttle
Copy link
Owner

harttle commented Feb 22, 2019 •
edited
Loading

Since it'll introduce many micro-tasks (maybe slower) to support this feature, I' d rather make it pending until we get benchmark ready. I opened a issue #105 to get it rolling.

@harttle harttle mentioned this issue Mar 1, 2019
Closed
@harttle harttle added this to the 8.0.0 milestone Mar 9, 2019
harttle added a commit that referenced this issue Mar 10, 2019
@harttle
feat: promise support for drops, working on #65
4a8088d
harttle pushed a commit that referenced this issue Mar 10, 2019
@semantic-release-bot
chore(release): 7.6.0 [skip ci]
834e50f 
# [7.6.0](v7.5.1...v7.6.0) (2019-03-10)

### Features

* promise support for drops, working on [#65](#65) ([4a8088d](4a8088d))
harttle pushed a commit that referenced this issue Mar 10, 2019
@semantic-release-bot
chore(release): 8.0.0 [skip ci]
84ea2c7 
# [8.0.0](v7.5.1...v8.0.0) (2019-03-10)

### Code Refactoring

* use camelCase for JavaScript APIs ([64e0c87](64e0c87))

### Features

* promise support for drops, working on [#65](#65) ([4a8088d](4a8088d))

### BREAKING CHANGES

* Options and method names in JavaScript API are now renamed to cammelCase, for a complete list see #109
@harttle
Copy link
Owner

harttle commented Mar 10, 2019

It's supported on 8.0.0. Refer to this test case

@harttle harttle closed this as completed Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking change feature requested
Projects
None yet
Milestone
8.0.0
Development

No branches or pull requests
2 participants
@cmawhorter @harttle