On Tue, Jul 11, 2017 at 10:18 PM, Robert Eckhardt <reckha...@pivotal.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.p...@enterprisedb.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.ku...@enterprisedb.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.ku...@enterprisedb.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.p...@enterprisedb.com> >>>> wrote: >>>> >>>>> Nice! >>>>> >>>>> On Tue, Jul 11, 2017 at 9:42 AM, Neel Patel < >>>>> neel.pa...@enterprisedb.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.ku...@enterprisedb.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.ku...@enterprisedb.com> wrote: >>>>>>> >>>>>>>> >>>>>>>> On Tue, Jul 11, 2017 at 1:22 PM, Dave Page <dp...@pgadmin.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.ku...@enterprisedb.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 < >>>>>>>>>> smcal...@pivotal.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 >> >
