Re: [pgadmin-hackers] [pgAdmin4] [PATCH] History Tab rewrite in React

From: George Gelashvili <ggelashvili(at)pivotal(dot)io>
To: Dave Page <dpage(at)pgadmin(dot)org>
Cc: Surinder Kumar <surinder(dot)kumar(at)enterprisedb(dot)com>, Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>, Shruti B Iyer <siyer(at)pivotal(dot)io>, Joao Pedro De Almeida Pereira <jdealmeidapereira(at)pivotal(dot)io>, Murtuza Zabuawala <murtuza(dot)zabuawala(at)enterprisedb(dot)com>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>, Sarah McAlear <smcalear(at)pivotal(dot)io>
Subject: Re: [pgadmin-hackers] [pgAdmin4] [PATCH] History Tab rewrite in React
Date: 2017-06-20 21:29:23
Message-ID: CAHowoHbcOZviGVDvu1zzsaeH0EYNq+t-pUo3cba=V8FFCaUikg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

We learned that the underlying issue was related to react-dom's
SyntheticEvent.augmentClass function being undefined.

This seems to be caused by attempted property assignment after the
SyntheticEvent had been replaced by a Proxy of itself. This works fine in
Chromium et al, but QtWebKit doesn't deal with Proxy Event objects well.
Moving the augmentClass definition and assignment up above the Proxy stuff
resolves the issue in a PR to React:
https://github.com/facebook/react/pull/10011

This has a decent chance of being rejected, as QtWebKit appears to be
losing support.

While and if this is being sorted out, we vendorized React, as one does.
Here are patches on the version we were using previously, and the fix from
the above PR.

We started talking to some Pivotal folks about QtWebKit to see if we can
fix the Proxy Event issue in the browser.
We are also looking into replacements for QtWebKit.

Thanks!
George and Sarah

On Tue, Jun 13, 2017 at 7:19 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> On Tue, Jun 13, 2017 at 11:47 AM, Surinder Kumar
> <surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
> > On Tue, Jun 13, 2017 at 4:05 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> >>
> >> On Tue, Jun 13, 2017 at 11:31 AM, Surinder Kumar
> >> <surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
> >> >
> >> > On Tue, Jun 13, 2017 at 3:56 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> >> >>
> >> >> On Tue, Jun 13, 2017 at 11:22 AM, Ashesh Vashi
> >> >> <ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
> >> >> > On Tue, Jun 13, 2017 at 2:47 PM, Dave Page <dpage(at)pgadmin(dot)org>
> wrote:
> >> >> >>
> >> >> >> And then I find a problem. Sigh.
> >> >> >>
> >> >> >> When running in the desktop runtime, under QtWekKit (the forked,
> >> >> >> updated version that is by far the best of the browser engines
> we've
> >> >> >> used), we get the attached error at startup. I don't see this
> under
> >> >> >> QtWebEngine, though as we've already found, that's not usable for
> >> >> >> other reasons.
> >> >> >>
> >> >> >> Is this fixable?
> >> >> >
> >> >> > As per 'http://qtwebkit.blogspot.in/2016/08/qtwebkit-im-back.html'
> :
> >> >> > "
> >> >> > WebKit engine itself has not been updated since Qt 5.2 release.
> >> >> > That's
> >> >> > why
> >> >> > it didn't support recent changes in Web standards that happened
> after
> >> >> > 2013,
> >> >> > including: new JavaScript language standard ES2015 (also known as
> >> >> > ES6),
> >> >> > as
> >> >> > well as improvements in DOM API and CSS.
> >> >> > ...
> >> >> > "
> >> >> >
> >> >> > Could this be a reason?
> >> >>
> >> >> For the old webkit, certainly, but if you read further down, the
> >> >> version we're using has been updated and does now claim to support
> >> >> most of ES2015.
> >> >
> >> > In fact the modern browsers don't support ES6 JS so the JS files
> >> > containing
> >> > ES6 code are first transpiled using babel into JS that browsers
> >> > supports.
> >>
> >> Well, we *could* do that, but are we? I'm not sure the current code
> >> does anything more than lint and webpack it.
> >
> > Yes. babel is used in webpack config which converts JSX, JS and react JS
> > library code into browser supportable JS code and put them into generated
> > directory which we are then imported in sqleditor.js
>
> Ahh, yes - I see it (still getting my head around this stuff). So,
> perhaps we need a different transform to support webkit?
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>

Attachment Content-Type Size
01-vendorize-react.diff text/plain 2.7 MB
02-react-dom-synthetic-event-fix.diff text/plain 843.4 KB

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Murtuza Zabuawala 2017-06-21 05:45:25 [pgAdmin4][PATCH] To fix the issue in Synonym node
Previous Message Shruti B Iyer 2017-06-20 20:39:35 [pgAdmin4][PATCH] Consolidating Selection Colors