Re: [pgAdmin4]: Webpacking of static JS/CSS

From: Neel Patel <neel(dot)patel(at)enterprisedb(dot)com>
To: Dave Page <dave(dot)page(at)enterprisedb(dot)com>
Cc: Dave Page <dpage(at)pgadmin(dot)org>, Sarah McAlear <smcalear(at)pivotal(dot)io>, George Gelashvili <ggelashvili(at)pivotal(dot)io>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>, Matthew Kleiman <mkleiman(at)pivotal(dot)io>, Surinder Kumar <surinder(dot)kumar(at)enterprisedb(dot)com>
Subject: Re: [pgAdmin4]: Webpacking of static JS/CSS
Date: 2017-07-11 08:42:53
Message-ID: CACCA4P0iT+vYW+LUZygCKde3bydUdXRP=yk-TXVtJswKZ_gxAg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

Hi Dave,

I have tested Surinder's patch in my local machine with Qt 5.9.1 Webkit on
Windows and we are getting improvements with this patch. :)

Below performance tested with Qt 5.9.1 with annulen webkit v 5.212.0 Alpha2.

*Before Webpack patch:-*

It took ~20 seconds. This 20 seconds includes when user double click on
application ( timing of python server start + page load )

*After Webpack patch:-*

It took ~13 seconds ( timing of python server start + page load ).

We got ~7 seconds improvement with webpack on same machine & same Qt
configuration and this will be useful in windows performance issue as well
:)

Thanks,
Neel Patel

On Tue, Jul 11, 2017 at 1:27 PM, Surinder Kumar <
surinder(dot)kumar(at)enterprisedb(dot)com> wrote:

> Hi
>
> I found this patch won't work on IE, so i have fixed it and will send
> updated patch.
>
> On Tue, Jul 11, 2017 at 1:25 PM, Surinder Kumar <
> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>
>>
>> On Tue, Jul 11, 2017 at 1:22 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> When you say "un-vendorising", do you mean removing them from the vendor
>>> directory and adding them to packages.json so they're pulled in via
>>> npm/yarn? (if so, good :-) )
>>>
>> ​Yes.​
>>
>>>
>>> On Tue, Jul 11, 2017 at 7:34 AM, Surinder Kumar <
>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi
>>>>
>>>> *Detailed changes:*
>>>>
>>>> ​1) Created a file bundle/app.js which loads `browser.js` and then
>>>> 'tools.datagrid' and its other dependents are configured to load in
>>>> '`imports-loader`
>>>>
>>>> 2) bundle/codemirror.js - it generates a bundled JS which is used
>>>> wherever required in various modules.
>>>>
>>>> 3) file_utils.js - It bundles the file manager's utility JS file and
>>>> loaded from `file manager/index.html`
>>>>
>>>> 4) lib_css - It contains list of vendor CSS to be bundled.
>>>>
>>>> 5) pgadmin_css - It contains list of overrides CSS which are bundled
>>>> and which has to load after vendors CSS is loaded.
>>>>
>>>> *Various Webpack configuration parameters:*
>>>>
>>>> 1) externals: List of template files to be loaded dynamically when a
>>>> call is made by dependent modules. These files are excluded from the
>>>> bundled JS.
>>>>
>>>> 2) resolve > alias - This resolves the path of module JS which is
>>>> referred in other modules; For example:
>>>> 'backbone': path.join(__dirname, './node_modules/backbone/backbone')
>>>>
>>>> 3) `backbone` is used in 'define(['backbone'])' calls and its path is
>>>> resolved to above absolute path.
>>>>
>>>> 4) 'pgLibs': List of files to bundle into `pgadmin_commons.js`. The
>>>> purpose is to separate files other than browser node modules JS in
>>>> `pgadmin_commons.js` and browser node modules JS in `app.bundle.js`. It
>>>> will help in debugging the code during development.
>>>>
>>>> *Un-vendor modules:*
>>>>
>>>> All modules are un-vendored except:
>>>> - requireJS
>>>> - Backgrid JS - it gives reference error when importing backgrid.css
>>>> from node_modules in bundle/lib.css even if the path is correct. I logged
>>>> an issue:
>>>> Opened an issue
>>>> <https://github.com/webpack-contrib/css-loader/issues/567>
>>>>
>>>> - React JS - I didn't un-vendor React JS because the pivotal developers
>>>> submitted a patch to fix issue of QtWebEngine. Once the patch is merged in
>>>> React repo, we will un-vendor.
>>>>
>>>> *Other issues faced:*
>>>>
>>>> 1) Backbone JS: I didn't upgraded backbone JS to latest because it
>>>> affects the application code mainly `Preferences module`.
>>>>
>>>> 2) jQuery: I didn't upgraded it to latest because it is gives error in
>>>> loading wcIframe of wcDocker in Query tool. I submit a PR
>>>> <https://github.com/WebCabin/wcDocker/pull/118>.
>>>>
>>>> 3) Acitree - it is not available in yarn repository. logged an request
>>>> <https://github.com/dragosu/jquery-aciTree/issues/15>
>>>> However i am managing it on my github account by forking this repo.
>>>>
>>>> Specified the repo github link to `acitree` in package.json with tag to
>>>> pull from
>>>> 4.5.0-rc.7
>>>> <https://github.com/imsurinder90/jquery-aciTree.git#4.5.0-rc.7>. The
>>>> latest version of actiree has issues so code is used form 4.5.0-rc.7 tag.
>>>>
>>>> Thanks to bestander <https://github.com/bestander> for helping this
>>>> out. link to thread
>>>> <https://github.com/yarnpkg/yarn/issues/1747#issuecomment-312502008>
>>>>
>>>> *Plugins used:*
>>>>
>>>> ​- optimize-css-assets-webpack-plugin: its job is to optimise the CSS
>>>> bundle like minifying CSS and removing comments from it. [used only in
>>>> production]
>>>>
>>>> - uglifyJS - It minimise the bundled JS​, and remove console
>>>> statements from code. [used only in production]
>>>>
>>>> - definePlugin - It helps in minimising the `React' production bundle.
>>>> As react has conditional code based on 'NODE_ENV' variable. [used only in
>>>> production]
>>>>
>>>> - providePlugin - It makes the variable defined through out the
>>>> application context. For example: jQuery object can be accessed wherever
>>>> it is used but not included in `define` calls.
>>>>
>>>> - CommonsChunkPlugin - it helps in separating vendor like code, common
>>>> dependent modules used in multiple modules. it extracts out in a file.
>>>>
>>>> - extractTextPlugin - it is used in combination with 'css-loader' and
>>>> 'style-loader'. It helps in extracting CSS and moved into files.
>>>>
>>>> - webpack-bundle-analyzer - it helps in analysing the bundled JS files
>>>> like size of bundled JS and which JS files are included in bundle. It is
>>>> commented out. [Use only when need to analyse]
>>>>
>>>> *Loaders used:*
>>>>
>>>> ​- shim-loader: It manages the module dependency, uses the same
>>>> configuration used in ​requireJS
>>>>
>>>> - imports-loader: It also used to loaded dependent modules which are
>>>> defined in its `use` setting.
>>>>
>>>> - file-loader - It helps in extracting font, image files into a
>>>> separate directory.
>>>>
>>>> - css-loader - Imports css files included in modules using `require`
>>>> and `import` calls.
>>>>
>>>> *TODO:*
>>>>
>>>> ​1) Automatically handle static and template JS files: This is already
>>>> being discussed. Once it is sorted out, we will change webpack
>>>> configuration accordingly.
>>>>
>>>> 2) Implementing Caching: I will look into this once an initial patch is
>>>> commited. and later on add as improvement.
>>>>
>>>> 3) Source maps: It will help in debugging bundled JS code in production
>>>> environment.
>>>>
>>>> 4) Feature tests are failing: I am currently looking into it. Query
>>>> tool functionality is working fine, the issue is it is unable to find code
>>>> mirror.
>>>>
>>>> *Steps to run:*
>>>>
>>>> ​After applying patch:* git apply --binary /path/to/patch*
>>>>
>>>> run* `yarn install`*
>>>> then run:
>>>>
>>>> *​*In development mode:
>>>> *yarn run bundle:dev*
>>>>
>>>> In production mode:
>>>> *yarn run bundle:prod​*​
>>>>
>>>> *Performance comparison:*
>>>>
>>>> On Mac's Chrome - Before bundle it was taking ~9sec to load page. After
>>>> bundle it took 3.5secs (with no cache)
>>>>
>>>> Please review the patch.
>>>>
>>>> Thanks,
>>>> Surinder
>>>>
>>>>
>>>> On Wed, Jul 5, 2017 at 8:22 PM, Sarah McAlear <smcalear(at)pivotal(dot)io>
>>>> wrote:
>>>>
>>>>> Hello,
>>>>>
>>>>>
>>>>>> *​Things to discuss:*
>>>>>>
>>>>>> How to differentiate between a static and template JS
>>>>>> ​​
>>>>>> .
>>>>>>
>>>>>
>>>>> What is the advantage of webpacking templated JS? It seems as though
>>>>> this creates a system in which the bundled dependencies have to refer back
>>>>> to the backend to load the templates.
>>>>>
>>>>> If there is a performance win in packing templated JS then looking at
>>>>> it makes sense. Otherwise it may make sense to put off until it is clear
>>>>> that the templated files should be dealt with by either de-templating them
>>>>> or bundling them where there is a clear reason.
>>>>>
>>>>> However, we're wondering about possible performance penalties with
>>>>> templating larger files (as opposed to templating on-demand.) Since jinja
>>>>> templates can execute arbitrary python, this could get time expensive and
>>>>> further slow things like initial page-load.
>>>>> Another concern is: what happens when a template gets out of date
>>>>> (e.g. if browser.js had previously filled in the content for
>>>>> 'panel_item.content' and had been cached, would it render a new version
>>>>> with the new values when needed? Or is it possible that we would get old
>>>>> content?)
>>>>>
>>>>>
>>>>>> *Taks remaining:*
>>>>>>
>>>>>> ​1. ​
>>>>>> Fix local variables which are declared without using var, have to
>>>>>> check in each file
>>>>>> ​ by​
>>>>>> running eslint (For now, i will fix only errors which are giving
>>>>>> error in browser).
>>>>>>
>>>>>> ​2. ​
>>>>>> Move non-template files from ’templates’ to ’static’ directory. List
>>>>>> of
>>>>>> ​ pending​
>>>>>> modules is here:
>>>>>>
>>>>>> - Tools (mostly all modules - 9 modules)
>>>>>> - Browser nodes - 3 modules(resource group, roles, tablespace)
>>>>>> - ​About
>>>>>> ​​
>>>>>>
>>>>>> Also can we move
>>>>>> ​'​
>>>>>> dashboard, statistic
>>>>>> ​s​
>>>>>> , preferences and help
>>>>>> ​'​
>>>>>> modules inside misc to preserve modularity as pgAdmin is modular
>>>>>> ​ ?​
>>>>>>
>>>>>
>>>>> Is there anything from a organization stance you discussed in the
>>>>> previous email that needs to be done to make this usable and consistent?
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> George & Sarah
>>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>
>

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message McDonaldR 2017-07-11 09:03:18 RE: Windows testing required: Updated runtime
Previous Message Dave Page 2017-07-11 08:40:53 Re: [pgadmin-hackers] [Design update] Style guide for pgAdmin4