Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dIcEj-0008JS-SL for pgadmin-hackers@arkaria.postgresql.org; Wed, 07 Jun 2017 14:47:06 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1dIcEj-0001NV-9x for pgadmin-hackers@arkaria.postgresql.org; Wed, 07 Jun 2017 14:47:05 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1dIcEi-0001NI-GS for pgadmin-hackers@postgresql.org; Wed, 07 Jun 2017 14:47:04 +0000 Received: from mail-io0-x236.google.com ([2607:f8b0:4001:c06::236]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1dIcEd-0005rE-DF for pgadmin-hackers@postgresql.org; Wed, 07 Jun 2017 14:47:03 +0000 Received: by mail-io0-x236.google.com with SMTP id y77so7882897ioe.3 for ; Wed, 07 Jun 2017 07:46:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Lu6kvXDhvASaUJBwWfymdvH9kLkj1u7rEZyq6rCJ6rs=; b=gC2MY+LTTWGLMzKqJlEbcSniLFIUQsWBKm4lAiQ/YIIu0ingBwa+vXQKNm4M4CUojY Bdk29ta/IFttB0cOGSTc6njjUr5ihcNKOcbN2W6OAT85rtT0sgdhNaEh6G/6awfciW7i mhsxltJkPi3C1leER3aGDv6UEEBPDT30NXxHCS8/MXBjXTdx1IOgBNV2eRwVahbSQUGU KspzgZZm/SJn5GDHQSKq8mxi9sUWfMLMnlhvXUAgguDQFeuvZETmOl8WHRq1iDfrHBfI bMfCKC50OR9EZEag6OUCiAqtQPHatgbLBClzBH1mOsHJLzzVQcH1Q+VQ3COZkesQNaPI ClLQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Lu6kvXDhvASaUJBwWfymdvH9kLkj1u7rEZyq6rCJ6rs=; b=rZZ1tuKQQJUyX/g5zksyd8ZRPuTMrhrzrltmNFebLf3JYEwAp1SGs71omX6KvrACEc 0UzsyKG26LMeXDk9S4Eok9GLashEoPGchhg7JxC0QRBNTWzm/8WvdP2iIu8J6HGHx4A4 uayKOSCxlPJ6fqYjP5h5Ppel8KjLgGsKgMuijSYQ3sQQ8wO3NmssELub8AgH6s5z1Tbb gZcFDKqTb37ObZ1gM4YoJi+vnvoPLGyi/nDQ/Q6YT88I7uUW+LrlWZF8U4WfLtVtOgpt mhQ3XMh/ptlceJ8pJEqIMnsSGW7kA0bFx8t12042u+aX50O1Fn04HLb5XVHV3Y+Oj8jr xvZQ== X-Gm-Message-State: AODbwcDLbsk5nu9HrbBVgQViaJlqxYbragR4wB9wwLBmBFLAoUMAVdzo M5Go8iA6dOV0qC41vYV0Y7UlLsM15M3F X-Received: by 10.107.35.210 with SMTP id j201mr31047149ioj.144.1496846817419; Wed, 07 Jun 2017 07:46:57 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.174.147 with HTTP; Wed, 7 Jun 2017 07:46:56 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Wed, 7 Jun 2017 15:46:56 +0100 Message-ID: Subject: Re: [pgAdmin4] [PATCH] History Tab rewrite in React To: Sarah McAlear Cc: Surinder Kumar , Matthew Kleiman , Joao Pedro De Almeida Pereira , Murtuza Zabuawala , pgadmin-hackers Content-Type: text/plain; charset="UTF-8" X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org Hi On Tue, Jun 6, 2017 at 9:15 PM, Sarah McAlear 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 > 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 >> 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 >>> wrote: >>>> >>>> On Wed, May 31, 2017 at 12:01 AM, Joao Pedro De Almeida Pereira >>>> 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 wrote: >>>>>> >>>>>> Hi >>>>>> >>>>>> On Tue, May 30, 2017 at 2:47 PM, Matthew Kleiman >>>>>> 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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers