Thread: [pgadmin-hackers] Re-vamping the history tab

Why anything new? Adding any library needs to be fully justified, and
adding one this large, that gives similar capabilities to one we
largely already have is going to require a significant amount of
convincing before it would be added.

Of course.  That's why we're starting this conversation early. :)

In this case I can't see any reason why this feature couldn't be quite
simply added using just jQuery to make Ajax calls to get the history,
and (possibly) get the history detail when a row is selected, assuming
it's decided that that info should be retrieved on demand rather than
all at once. Additional rows can be added to the UI in response to
query results being received (and of course, saved to the SQLite
database at the backend for future sessions to read).

 

In this case I don't even think using backbone.js (our existing MVC
library) would buy a huge amount, though it may be a little more
convenient.

We will use jquery to make ajax calls that fetch the history data, but actually rendering that data and managing the data once it has been fetched will be handled with React. 

Backbone is not as powerful as React, and does not provide the ease of ensuring a consistent state:
for example, which tree node is expanded and which context menu applies to each node.

Granted, the history tab has less state to manage than the tree. It still has some:
which row is highlighted in the left panel and which data is displayed in the right.

That's part of why I think the history tab is a good place to start with React. it is a simple demonstration of the way React will drop in.

Backbone tends to leak memory over time unless you are very careful about unbinding events when re-rendering. So users switching between various items in history tab may have a degraded experience over time if we use backbone.

Using react will make it easier to avoid situations like the one that has developed with the server tree bug.

Pgadmin4 is a heavily client-side application, and its Javascript code needs to be treated as a first class citizen. The same principles of needing to be highly componentized, easily understood by future developers, and readily changeable should be applied to the front end code.

Here are some resources on react for the interested: 

React vs Backbone: http://www.code-experience.com/react-js-vs-traditional-mvc-backbone-ember-angular/

React vs Angular: https://medium.com/@paramsingh_66174/angularjs-vs-reactjs-e651a194dfcb#.oopgbfq3z

Another quick take on why react: https://www.syncano.io/blog/reactjs-reasons-why-part-1/

For review purposes, that's fine. Please ensure such patches are
clearly marked as WIP or for comment only so we know not to commit
them.

 

Sounds good! 

Please be careful to only create RMs for distinct new features, as
that list is used as the changelog for users so we need it to make
sense to them. They dont care about incremental changes or
infrastructure changes unless the fix a specific bug or add a specific
feature.

Ah! That's really useful clarification on how we should be using Redmine. Please continue to provide us feedback about whether our Redmine stories are at the right level.

Tira & George

On Thu, Mar 23, 2017, 5:17 AM Dave Page <dpage@pgadmin.org> wrote:

Hi

On Wed, Mar 22, 2017 at 7:09 PM, Atira Odhner <aodhner@pivotal.io> wrote:
> Hi Hackers,
>
> We want to give you a heads up on some of the changes we are planning and
> get your input on the implementation plan.
>
> The Redmine issue is here: https://redmine.postgresql.org/issues/2282
>
> Over the past month we have been doing many user interviews and one of the
> major asks is that the History tab could store the complete query text. From
> this feedback we started several threads on the design:
>
> https://www.postgresql.org/message-id/CAPG3WN7YxVO+n3qbDOWQBWY0rkscq1ar-5V-PJBSEPPZNsT_qw@mail.gmail.com
> https://www.postgresql.org/message-id/CAPG3WN6yLGkD8DkAvdO0MWnNXFEisU5UXJgrLX%3D-kcMnrwaT2A%40mail.gmail.com
>
> The user level, here are the changes we want are:
> Complete query text stored in the History tab
> Complete messages text stored in the History tab
> Redesign of the tab in order to allow for both of the above in an easily
> browsable format.

I've been impressed with the process that's taken us this far. It's worked well.

> Implementation Plan
>
> In order to accomplish this the plan is to rip out the existing History tab
> code, and rewrite it in React.
>
> Why React:
> React gives us better state management, an ability to ensure that we are
> rendering consistent state in the history feature. React is highly
> performant with a shadow-dom that re-renders only changed components. It is
> also modular and easy to create components that are self-contained and
> reusable.

Why anything new? Adding any library needs to be fully justified, and
adding one this large, that gives similar capabilities to one we
largely already have is going to require a significant amount of
convincing before it would be added.

In this case I can't see any reason why this feature couldn't be quite
simply added using just jQuery to make Ajax calls to get the history,
and (possibly) get the history detail when a row is selected, assuming
it's decided that that info should be retrieved on demand rather than
all at once. Additional rows can be added to the UI in response to
query results being received (and of course, saved to the SQLite
database at the backend for future sessions to read).

In this case I don't even think using backbone.js (our existing MVC
library) would buy a huge amount, though it may be a little more
convenient.

> Why rip out history as it exists:
> We want to test the history feature as we build it and it’s not currently
> built in a way that is testable. It is baked into sqleditor.js and not well
> isolated from other features.
>
> As previewed in the design, the setup is quite different and we believe it
> will be faster to re-write this feature than to make these changes in-place.

How you develop is up to you, however the master branch needs to
remain in a releasable state in case we have any security bugs
reported the necessitate a prompt release.

> Also:
> We will need a JS build step in order to pull in React (or any framework) as
> we do not want to add it as a vendorized dependency because of its size.
>
> Pre-feature-parity commits:
> Since we are re-writing this history feature, we will be submitting patches
> as we build it to regularly communicate our progress before we have one
> gigantic patch.

For review purposes, that's fine. Please ensure such patches are
clearly marked as WIP or for comment only so we know not to commit
them.

> Once we have achieved parity, we will create new redmine stories to bring in
> the additional improvements such as highlighting failed queries and making
> it easy to copy the historical query.

Please be careful to only create RMs for distinct new features, as
that list is used as the changelog for users so we need it to make
sense to them. They dont care about incremental changes or
infrastructure changes unless the fix a specific bug or add a specific
feature.

Thanks for your work on this!

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Mar 23, 2017 at 8:23 PM, Dave Page <dpage@pgadmin.org> wrote:

On Thu, Mar 23, 2017 at 1:51 PM, Atira Odhner <aodhner@pivotal.io> wrote:
>
> We will use jquery to make ajax calls that fetch the history data, but
> actually rendering that data and managing the data once it has been fetched
> will be handled with React.
>
> Backbone is not as powerful as React, and does not provide the ease of
> ensuring a consistent state:
> for example, which tree node is expanded and which context menu applies to
> each node.
>
> Granted, the history tab has less state to manage than the tree. It still
> has some:
> which row is highlighted in the left panel and which data is displayed in
> the right.
>
> That's part of why I think the history tab is a good place to start with
> React. it is a simple demonstration of the way React will drop in.

I think it's a poor example; firstly because those of us that will
have to review the code will have zero experience at that time, and
secondly because it doesn't really give us a good basis for
comparison.

A rewrite of a treeview node or two and something like the Grant
Wizard would likely be far more convincing, as they would allow us to
make an apples to apples comparison, and really understand the
practical differences between the two technologies.

I'd also want a committed plan to migrate - I do not want to end up in
a situation where we have a mish-mash of backbone and react for any
real period of time.

First impression of react from blogs, sites are looking promising.

And, I am not against changing it.

But - I agree with Dave.

Using the new technology with new feature will not give us the metrics for comparison.

> Backbone tends to leak memory over time unless you are very careful about
> unbinding events when re-rendering. So users switching between various items
> in history tab may have a degraded experience over time if we use backbone.

Agree - we need to be extra careful using the Backbone, when attaching the events.

Specially - when we attach them from the 'render' function.

(Also - there are many corner cases, when we need(ed) to be careful.)

We tried that the same with the current implementation.

I won't claim - it's completely leak proof.

But - we always improved over the period of time.

>
> Using react will make it easier to avoid situations like the one that has
> developed with the server tree bug.

Hmm.. I do not agree here.

Yes - there is a bug, and we need to fix it.

How does it make it easier to 

>
> Pgadmin4 is a heavily client-side application, and its Javascript code needs
> to be treated as a first class citizen. The same principles of needing to be
> highly componentized, easily understood by future developers, and readily
> changeable should be applied to the front end code.

True, we could have done better.

For sure - we also need to be mindful of bloat and having
half-implemented changes (which is really is my biggest issue here)
that we end up having to maintain for years to come. Worst case
scenario for me would be that your team starts a migration project,
whilst my team builds new features on that work, then for whatever
reason, the Pivotal team stops the work halfway through, leaving us
with a mess of code that we cannot easily fix without finding a bunch
of resources.

+1 

I will stress that I have no reason to expect Pivotal to do that, but
I have been burnt in that way by other (now defunct) Postgres
companies and recovery took years.

Agree with Dave here.

Backbone is tightly bound with the application at the moment, and replacing it will need lot of thoughts, consideration, planning, and commitment.

> Here are some resources on react for the interested:
>
> React vs Backbone:
> http://www.code-experience.com/react-js-vs-traditional-mvc-backbone-ember-angular/
>
> React vs Angular:
> https://medium.com/@paramsingh_66174/angularjs-vs-reactjs-e651a194dfcb#.oopgbfq3z
>
> Another quick take on why react:
> https://www.syncano.io/blog/reactjs-reasons-why-part-1/

It certains sounds like it would be a good move from a purely
technical perspective.

+1

-- Thanks, Ashesh

I absolutely need to hear opinions from the EDB
team though (CC'd).

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company