search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

Update Mustache -> 2.2.1 #13603

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by MarcelGerber Wednesday May 25, 2016 at 21:34 GMT Originally opened as https://github.com/adobe/brackets/pull/12455

This PR updates Mustache from version 0.7.2 to 2.2.1 This improves performance, adds new features and includes better error handling (full changelog).

Also, thanks to@petetnt for the initial PR (#12034), which he wanted replaced by this one.

MarcelGerber included the following code: https://github.com/adobe/brackets/pull/12455/commits

core-ai-bot commented 3 years ago

Comment by petetnt Thursday May 26, 2016 at 06:04 GMT

Thanks Marcel 👍

core-ai-bot commented 3 years ago

Comment by MarcelGerber Thursday May 26, 2016 at 09:43 GMT

@ingorichter I've tested the compared performance of this Mustache version in the Installed tab of the Extension Manager, and quite surprisingly, I saw a 13% decline in performance (1351ms -> 1534ms).

core-ai-bot commented 3 years ago

Comment by ingorichter Thursday May 26, 2016 at 20:47 GMT

I guess you meant ms instead of secs. ;-) Is this the result for the 2nd time you opened the dialog? I wonder, if this time is all spent in mustache.render()? Or is there anything else going on in our code, where we could improve the performance. Anyway, the performance hit is not that big and probably hardly noticeable, but there might be people on the team generally concerned with a performance degradation, when there is no other advantage of using a newer version of the the library.

core-ai-bot commented 3 years ago

Comment by MarcelGerber Thursday May 26, 2016 at 21:02 GMT

This is the result measured when opening the dialog the first time, but I instrumented it in such a way that only the rendering part is considered, not the downloading part. Well, there's quite some churn involved in the rendering; but the biggest mistake we make is that we render all the items (1187 at this point) at once, instead of rendering them on-demand.

core-ai-bot commented 3 years ago

Comment by ingorichter Friday Jun 10, 2016 at 00:50 GMT

The information underneath the extension name doesn't get properly evaluated. Do you see the same issue? image

core-ai-bot commented 3 years ago

Comment by MarcelGerber Friday Jun 10, 2016 at 12:38 GMT

To me, it looks like you didn't run git submodule update. After updating the Mustache submodule, it works fine for me :)

core-ai-bot commented 3 years ago

Comment by ingorichter Friday Jun 10, 2016 at 17:02 GMT

Yeah, that was missing. Thank you. Now it looks fine again. Coming back to the performance issue: it doesn't feel slower. The perceived performance was already slow before the update. The most important question for me is, how we can render the list on demand, without having a big impact on usability. What happens if the user searches? What if the user scrolls immediately to the bottom of the list? It would be great, if we could find a way to make it faster.

core-ai-bot commented 3 years ago

Comment by MarcelGerber Friday Jun 10, 2016 at 18:22 GMT

I agree, it does feel slow already. So, when searching, we should completely re-render the list, showing only the first few items (30 seems like a good magic number to me). When scrolling down, there should be a "Load more" button or just automatic appending of more items. We can kind of take app stores like the Google Play Store as an example.

Also, there's the awesome Polymer element iron-list which does exactly that, rendering only the visible DOM elements. CodeMirror, of course, does that too.

core-ai-bot commented 3 years ago

Comment by petetnt Friday Jun 24, 2016 at 10:36 GMT

We also got React as a dependency so virtual React lists would also be an easy & super fast solution.

One library as an example: https://github.com/developerdizzle/react-virtual-list

core-ai-bot commented 3 years ago

Comment by ingorichter Friday Aug 19, 2016 at 06:37 GMT

Auto appending sounds good to me. This will also solve the use case when the user scrolls immediately down to the bottom of the list.

core-ai-bot commented 3 years ago

Comment by swmitra Tuesday Aug 29, 2017 at 19:37 GMT

@MarcelGerber , I know it's a long time and we haven't taken any initiative to take this PR in, I am really sorry about that. Do we need to make one more update of the submodule? I would like to take this in 1.11 and we have very short schedule, is there any pending activity in here?