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

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.

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.

The new 2 patches are the same as before, without Grunt.

Thanks!
George & Sarah

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
>> <https://yarnpkg.com/lang/en/docs/cli/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
>>>>>
>>>>
>>>>
>>>
>>
>