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.

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

async version of models serializer #90

Closed
wants to merge 1 commit into from
Closed

async version of models serializer #90

wants to merge 1 commit into from

Conversation

uside
Copy link

@uside uside commented Jan 24, 2017

No description provided.

@coveralls
Copy link

coveralls commented Jan 24, 2017

Coverage Status

Coverage decreased (-1.4%) to 95.714% when pulling 6d69cb8 on uSide:develop into 7cad89a on adonisjs:develop.

@RomainLanz
Copy link
Member

RomainLanz commented Jan 24, 2017

Hey @uside!

Thanks for your contribution! ❤️

May you add some tests to your PR?


Previous discussion:

Vladimir Kleshko @uside Jan 23 17:21
hey guys! do you plan to implement computed properties with generators support?

Harminder Virk @thetutlage Jan 23 17:44
@uside Can u share the use-case on what exactly you are trying to achieve?

Vladimir Kleshko @uside Jan 23 18:38
for example, I want computed property for my model where I can fetch data from redis

@thetutlage
Copy link
Member

@uside I am not in favor of async computed properties. I asked for the use case to understand what exactly you are trying to achieve. Fetching data from redis is not a computed property at all and should be handled by your application.

@uside
Copy link
Author

uside commented Jan 24, 2017

@thetutlage okay, another simple example: I have some async service and want to retreive data from it without saving this data to db. Sure, this is not a "best practice" for large data, but for small selections it will be nice I think

@thetutlage
Copy link
Member

Yeah then you should make use of your controller to fetch the data, Why you want to keep it on the model instance?

@RomainLanz
Copy link
Member

RomainLanz commented Jan 24, 2017

Let me provide an example.

So you have your PostsController.index method where you want to display information about your posts.

{% for post in posts %}
  {# ... #}
  {{ post.view_count }}
  {# ... #}
{% endfor %}

The attribute view_count is stored in a Redis database, so the code above will not works.

You can do the same by using your controller to fetch the data from Redis and then you need to map the data.

{% for post in posts %}
  {# ... #}
  {{ redisData[post.id] }}
  {# ... #}
{% endfor %}

@uside
Copy link
Author

uside commented Jan 24, 2017

@thetutlage to reduce number of cycles and make code cleaner. If you have some tree data it will be a big controller like: load data from db -> call toJSON, where lucid map all relations/etc -> load data from async service -> another big map to extend result json. In this case we dont have code in model, but in result we will have a heavy controller

@thetutlage
Copy link
Member

thetutlage commented Jan 24, 2017

@RomainLanz Yes this is less expensive than hitting redis for each record that you fetch from database.

For example: If you have 30posts inside database, the computed property will make 30 calls to redis server for each post instance.

Also if their is complexity in the code, it is better to abstract that logic to usable service, instead of replying on computed properties.

Plus using lodash you can easily map things out, without hitting the performance bottlenecks

@RomainLanz
Copy link
Member

RomainLanz commented Jan 24, 2017

@uside Doing asynchronous task in a loop is not recommended. You will end up with a O(n) issue.

As @thetutlage said, if you have 30 posts you will need to do 30 requests to your service/database. That's why we eager/lazy-load relation.

You need to fetch those remote data (with one request if possible) from your controller and create a dictionary/hash to retrieve them in your view.

I have written an example above.

@uside
Copy link
Author

uside commented Jan 24, 2017

I have no more arguments. Close a pr ?

@thetutlage
Copy link
Member

If you feel your code is getting unmanagable, feel free to throw the actual situation you are in the gitter room and we all can discuss about it.

But async computed properties will case performance issues no matter what. Closing the PR for now

@thetutlage thetutlage closed this Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants