elephpant.me icon indicating copy to clipboard operation
elephpant.me copied to clipboard

perf: only execute database query when needed

Open svenluijten opened this issue 6 years ago • 4 comments

This ensures the CountriesQuery is only executed when the user actually lands on/loads any of the composed views. Previously, this would always execute the query (on every request) because it was done outside of the closure.

svenluijten avatar Oct 08 '19 13:10 svenluijten

we have pros and cons with both approaches as I can see.

  1. keeping the query outside the closure requires a single call on each request, and all views using it will have it already resolved, correct? so the worst case we will have a single query per request

  2. moving it to inside the closure will make the query to be made only on request that really needs to use countries (good), but if we have on the same request more than one view (or partial) using the countries I guess it will query it more than one time, correct? so I think on a @include('_user') call we might have multiples queries for the # of partials required we have

let me know what are your thoughts here please 💪 thank you so much

jgrossi avatar Oct 09 '19 15:10 jgrossi

Ah good points @jgrossi! I'm actualy not sure if Laravel already caches the result of a view composer, I'll have a look at the internals later and report back 😉

If Laravel indeed doesn't cache those results, what do you think of using spatie/once for this? Or would you rather we not pull in another dependency just for memoization? We could solve this pretty easily ourselves by "caching" the result of the query on a static class property.

svenluijten avatar Oct 10 '19 07:10 svenluijten

both ideas are good. spatie/once also uses static properties to cache the result in the same request. for me no problem of adding the package to composer. go ahead

jgrossi avatar Oct 13 '19 02:10 jgrossi

Hey guys, what's missing to merge this PR?

IgorDuarteDev avatar Sep 24 '20 12:09 IgorDuarteDev