Re: [pgAdmin4] [PATCH] History Tab rewrite in React

Hi Dave,

I split the linting out into an intermediate commit, and rebased on top of
master.

Please see attached.

Thanks!
George

On Wed, Jun 7, 2017 at 10:46 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
> On Tue, Jun 6, 2017 at 9:15 PM, Sarah McAlear <smcalear(at)pivotal(dot)io> wrote:
> > Hello,
> >
> >> First patch contains changes related to copy row feature which is
> already
> >> in process in another thread. Any reason to add those changes in this
> patch
> >> as this patch is meant to contain infrastructure changes ?
> >
> >
> > As part of introducing React, we are bringing a linter into the codebase.
> > The changes to the file you're referring to are formatting changes
> enforced
> > by the linter.
>
> Right - but they also make it a pain to review the code and see what
> has actually changed vs. what the linter has reformatted. Can we
> separate those changes out easily?
>
> >> Can the changes related to FeatureTest or Jasmine test be separated out
> in
> >> another patch?
> >
> >
> > Great question! Since we test-drove this, the value delivered by each
> > individual test represents a behavior. Each piece of functionality in the
> > implementation is directly related to their corresponding test(s).
> > In this way, the tests belong with the code in the same way as
> > documentation.
>
> I totally agree with that. We should always strive to have code, docs
> and tests together.
>
> > The new 2 patches are the same as before, without Grunt.
>
> Unfortunately they don't apply cleanly, and break the feature tests:
>
> With the first patch only:
>
> - The patch includes yarn.lock, which is a gitignore'd file.
> - karma.conf.js needed manual merging of every hunk.
> - When the feature tests run, the browser just flickers whilst it
> continually tries to connect to the server, all the time getting
> ERR_CONNECTION_REFUSED.
>
> Patch number 2 fails to apply on karma.conf.js, sqleditor.js,
> pgadmin_page.py and yarn.lock (which shouldn't be there anyway).
>
> Can you fix/rebase please?
>
> > On Mon, Jun 5, 2017 at 1:30 AM, Surinder Kumar
> > <surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
> >>
> >> Hi
> >>
> >> Review comments:
> >>
> >> 1) First patch contains changes related to copy row feature which is
> >> already in process in another thread. Any reason to add those changes in
> >> this patch as this patch is meant to contain infrastructure changes ?
> >>
> >> 2) Can the changes related to FeatureTest or Jasmine test be separated
> out
> >> in another patch?
> >>
> >>
> >> Thanks,
> >> Surinder
> >>
> >>
> >> On Thu, Jun 1, 2017 at 1:59 AM, Matthew Kleiman <mkleiman(at)pivotal(dot)io>
> >> wrote:
> >>>
> >>> Hey Surinder,
> >>>
> >>>> Is it possible to run the task listed above by configuring them in
> >>>> "scripts" in package.json file and running concurrently ?
> >>>
> >>>
> >>> That would be cleaner than what we proposed. We could use the yarn run
> >>> feature to do just what you are suggesting. It works the same as in
> npm.
> >>> Something like the following:
> >>> In package.json -
> >>> "scripts": {
> >>> "linter": "eslint pgadmin/static/jsx/**/*.jsx
> >>> pgadmin/static/js/selection/*.js regression/javascript/**/*.jsx
> >>> regression/javascript/**/*.js *.js"
> >>> }
> >>>
> >>> To call it on the command line -
> >>> yarn run linter
> >>>
> >>> - Matt
> >>>
> >>> On Wed, May 31, 2017 at 12:35 AM, Surinder Kumar
> >>> <surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
> >>>>
> >>>> On Wed, May 31, 2017 at 12:01 AM, Joao Pedro De Almeida Pereira
> >>>> <jdealmeidapereira(at)pivotal(dot)io> wrote:
> >>>>>>
> >>>>>> The motivation is simple - we want a solution that works for the
> whole
> >>>>>> app, can handle debug vs. release execution, pluggable modules, and
> >>>>>> installations in read-only directories.
> >>>>>
> >>>>> With the current configuration of Grunt, all the requirements you
> >>>>> mention are available.
> >>>>> The tasks on Grunt should only be run under development or before we
> >>>>> are creating the installer.
> >>>>
> >>>>
> >>>>>
> >>>>> The installer should pick up only the bundled Javascript and should
> >>>>> install it in the correct folders, this way there is no need to
> rewrite or
> >>>>> process files in read-only directories of the end user machine.
> >>>>>
> >>>>>> Per the other thread on the subject (that Joao suggested continuing
> >>>>>> discussion on), Surinder is currently looking into flask-webpack. I
> >>>>>> spent some time playing with grunt and some other options last week.
> >>>>>
> >>>>> We should continue the discussion about flask-webpack or Grunt or any
> >>>>> other Build system in the other thread, but we need to have some
> decision
> >>>>> because this patch needs a build system.
> >>>>> If we remove Grunt from this patch we will need to execute the
> >>>>> following command every time the application run:
> >>>>>
> >>>>> yarn run eslint pgadmin/static/jsx/**/*.jsx
> >>>>> pgadmin/static/js/selection/*.js regression/javascript/**/*.jsx
> >>>>> regression/javascript/**/*.js *.js && yarn run webpack -- --config
> >>>>> webpack.config.js && python web/pgAdmin4
> >>>>>
> >>>>> And to run the jasmine tests:
> >>>>>
> >>>>> yarn run eslint pgadmin/static/jsx/**/*.jsx
> >>>>> pgadmin/static/js/selection/*.js regression/javascript/**/*.jsx
> >>>>> regression/javascript/**/*.js *.js && yarn run karma start
> >>>>>
> >>>>> And to run the feature tests:
> >>>>>
> >>>>> yarn run eslint pgadmin/static/jsx/**/*.jsx
> >>>>> pgadmin/static/js/selection/*.js regression/javascript/**/*.jsx
> >>>>> regression/javascript/**/*.js *.js && yarn run webpack -- --config
> >>>>> webpack.config.js && python runtests.py
> >>>>>
> >>>>> Is this a solution that is acceptable?
> >>>>
> >>>>
> >>>> As per my knowledge, Grunt can run the tasks and bundle the JS and CSS
> >>>> files (using r.js which is not preferred). But Webpack can not do
> both. It
> >>>> can only bundle files, it has to be used with either Grunt or Gulp to
> >>>> execute tasks.
> >>>>
> >>>> Is it possible to run the task listed above by configuring them in
> >>>> "scripts" in package.json file and running concurrently ?
> >>>>
> >>>> scripts: {
> >>>> "task-name1": "yarn run eslint filename",
> >>>> "task-name2": "yarn run karma start", so on...
> >>>> }
> >>>>
> >>>> and run them using following command:
> >>>> npm run task-name1
> >>>>
> >>>>>
> >>>>>
> >>>>>> However; this patch is supposed to be about the history tab rewrite.
> >>>>>> Whatever solution we use for webpacking/transpiling/
> linting/minifying
> >>>>>> etc, it should be a standalone change as it's decidedly non-trivial.
> >>>>>
> >>>>> We split this patch into 2 different commits:
> >>>>> 1 - Add React and all the tools needed to work with it, this includes
> >>>>> Grunt, Webpack and ESLint
> >>>>> 2 - Change the history tab
> >>>>>
> >>>>> Do you want a single commit for each of the following?
> >>>>> React (just adds the React library via yarn)
> >>>>> Webpack (adds Webpack library via yarn and creates the
> >>>>> webpack.config.js)
> >>>>> ESLint (adds ESLint library via yarn, creates .eslintrc config
> file,
> >>>>> and corrects all lint errors that previously existed)
> >>>>> Grunt (adds Grunt library via yarn and creates Gruntfile.js config,
> >>>>> creating a pipeline of the previous three libraries/tasks)
> >>>>> Our change to History
> >>>>>
> >>>>> Thanks
> >>>>> Joao & Matt
> >>>>>
> >>>>> On Tue, May 30, 2017 at 10:10 AM, Dave Page <dpage(at)pgadmin(dot)org>
> wrote:
> >>>>>>
> >>>>>> Hi
> >>>>>>
> >>>>>> On Tue, May 30, 2017 at 2:47 PM, Matthew Kleiman <
> mkleiman(at)pivotal(dot)io>
> >>>>>> wrote:
> >>>>>> > Hi Dave,
> >>>>>> >
> >>>>>> > We are currently using the Grunt taskrunner to run the following
> >>>>>> > tasks:
> >>>>>> >
> >>>>>> > lint the javascript code
> >>>>>> > start javascript tests
> >>>>>> > invoke webpack to transpile and bundle js and jsx files
> >>>>>> > minify javascript
> >>>>>> >
> >>>>>> > In order to remove Grunt from the application, we will need some
> >>>>>> > other tool
> >>>>>> > to execute these listed tasks.
> >>>>>> >
> >>>>>> > There are other tools available, like Gulp.js, that we could use
> >>>>>> > instead of
> >>>>>> > Grunt. We would like to understand your motivation for removing
> >>>>>> > Grunt so we
> >>>>>> > can find a better solution.
> >>>>>>
> >>>>>> The motivation is simple - we want a solution that works for the
> whole
> >>>>>> app, can handle debug vs. release execution, pluggable modules, and
> >>>>>> installations in read-only directories.
> >>>>>>
> >>>>>> Per the other thread on the subject (that Joao suggested continuing
> >>>>>> discussion on), Surinder is currently looking into flask-webpack. I
> >>>>>> spent some time playing with grunt and some other options last week.
> >>>>>>
> >>>>>> However; this patch is supposed to be about the history tab rewrite.
> >>>>>> Whatever solution we use for webpacking/transpiling/
> linting/minifying
> >>>>>> etc, it should be a standalone change as it's decidedly non-trivial.
> >>>>>>
> >>>>>> --
> >>>>>> Dave Page
> >>>>>> Blog: http://pgsnake.blogspot.com
> >>>>>> Twitter: @pgsnake
> >>>>>>
> >>>>>> EnterpriseDB UK: http://www.enterprisedb.com
> >>>>>> The Enterprise PostgreSQL Company
> >>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
>
>
>
> --
> 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
>