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

On Tue, Jul 11, 2017 at 10:18 PM, Robert Eckhardt <reckhardt(at)pivotal(dot)io>
wrote:

> The last email on this chain from Surinder says that it isn't working on
> IE and he will submit another patch. Are we missing that patch? Would you
> like us to look at the previous patch?
>
​Yes previous patch includes fix related to IE.​

>
> -- Rob
>
>
> On Jul 11, 2017 11:37 AM, "Dave Page" <dave(dot)page(at)enterprisedb(dot)com> wrote:
>
>> Pivotal team; you guys are far more familiar with webpack etc. than we
>> are; could you review please?
>>
>> Thanks!
>>
>> On Tue, Jul 11, 2017 at 4:24 PM, Surinder Kumar <
>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>
>>> Hi
>>>
>>> 1) Created a separated patch for Un-vendored libraries and another one
>>> related to webpack bundle files so it is easy to review.
>>>
>>> 2) Removed commented out code and dead code.
>>>
>>> 3) Feature test cases are fixed. All are running.
>>> I have to add `time.sleep` of 1 second in method
>>> 'fill_codemirror_area_with' as sometimes it work and sometimes don't.
>>>
>>> 4. Removed unused libraries from package.json
>>>
>>> Please find updated patch and review.
>>>
>>> Thanks,
>>> Surinder
>>>
>>>
>>>
>>> On Tue, Jul 11, 2017 at 3:11 PM, Surinder Kumar <
>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi
>>>>
>>>> This patch doesn't work in windows IE 11 due to error `'Promise' is
>>>> undefined
>>>> ​`​.
>>>>
>>>> The dependency package 'babel-polyfill' is required to run ES6 code
>>>> with webpack and has to load before at entry point of app.
>>>> related thread <https://github.com/webpack/webpack/issues/4254>
>>>>
>>>> Please find updated patch.
>>>>
>>>> Thanks
>>>> Surinder
>>>>
>>>> On Tue, Jul 11, 2017 at 2:13 PM, Dave Page <dave(dot)page(at)enterprisedb(dot)com>
>>>> wrote:
>>>>
>>>>> Nice!
>>>>>
>>>>> On Tue, Jul 11, 2017 at 9:42 AM, Neel Patel <
>>>>> neel(dot)patel(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> 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/backb
>>>>>>>>>> one')
>>>>>>>>>>
>>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Dave Page
>>>>> VP, Chief Architect, Tools & Installers
>>>>> EnterpriseDB: http://www.enterprisedb.com
>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>> Blog: http://pgsnake.blogspot.com
>>>>> Twitter: @pgsnake
>>>>>
>>>>
>>>>
>>>
>>
>>
>> --
>> Dave Page
>> VP, Chief Architect, Tools & Installers
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>