Config.js modularized #14470

Piyush3079 · 2018-07-08T14:46:15Z

This pr contains the modular code for Config.js file. This file is used for settings on various pages.

This pr is in continuation of Mod_Jd_Import_Export. Before reviewing this pr, review Mod_Js_Import_Export and merge and then compare with this pr.
Before submitting pull request, please check that every commit:

Has proper Signed-Off-By
Has commit message which describes it
Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
Any new functionality is covered by tests

devenbansod · 2018-08-01T11:19:10Z

Please rebase and squash

Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>

Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>

devenbansod

Some minor comments and queries.

devenbansod · 2018-08-02T12:28:52Z

js/src/config.js

+// default values for fields
+// export var defaultValues = {};
+
+// ------------------------------------------------------------------


Is this code needed?

devenbansod · 2018-08-02T12:29:44Z

libraries/classes/Config/FormDisplayTemplate.php

@@ -483,7 +483,7 @@ public function addJsValidate($fieldId, $validators, array &$jsArray): void
                $vArgs[] = Sanitize::escapeJsString($arg);
            }
            $vArgs = $vArgs ? ", ['" . implode("', '", $vArgs) . "']" : '';
-            $jsArray[] = "validateField('$fieldId', '$vName', true$vArgs)";
+            $jsArray[] = "['$fieldId', '$vName', true$vArgs]";


Any reason why this is required?

This function is not available in global scope so it is giving reference error. There fore it is removed and an array is returrned from the function instead of a function call.

devenbansod · 2018-08-02T12:30:01Z

js/src/variables/get_config.js

+    }
+    $.extend(PMA_messages, arguments[arguments.length - 2]);
+    $.extend(defaultValues, arguments[arguments.length - 1]);
+    // console.log(PMA_messages);


stray debug statement?

devenbansod · 2018-08-02T12:30:04Z

js/src/variables/get_config.js

+}
+
+window.getConfigData = function () {
+    // debugger;


stray debug statement?

devenbansod · 2018-08-02T12:30:12Z

js/src/functions/config.js

+}
+
+export function setupValidation () {
+    // debugger;


stray debug statements?

Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>

…onfig.js in file loading object. Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>

Build script for js files. Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>

Piyush3079

Requesting review from @devenbansod

Piyush3079 · 2018-08-02T15:27:22Z

package.json

@@ -7,7 +7,7 @@
  "license": "GPL-2.0",
  "scripts": {
    "dev:wds": "webpack-dev-server --progress",
-    "prod:build": "del js/lib js/dist && babel js/src/ -d js/lib --ignore .test.js && cross-env NODE_ENV=production webpack -p --progress",
+    "prod:build": "del js/lib js/dist && babel js/src/ -d js/lib --ignore .test.js && cross-env NODE_ENV=production webpack -p --env.production --progress",


Adding production environment variable in webpack.config file.

Piyush3079 · 2018-08-02T15:27:58Z

scripts/build-js-files

@@ -0,0 +1,39 @@
+#!/bin/sh


@devenbansod can you please review this file. This is the script for creating production build for js files.

Consider renaming this as a build-js.sh script?

Also, try to put the invocation for this script in its proper place inside scripts/create-release.sh?

Piyush3079 · 2018-08-02T15:28:34Z

webpack.config.babel.js

@@ -2,84 +2,95 @@ import path from 'path';
 import webpack from 'webpack';


Update webpack.config file for creating production build using command line.

codecov · 2018-08-02T15:45:04Z

Codecov Report

❗ No coverage uploaded for pull request base (gsoc-js-refactoring@5e9d705). Click here to learn what that means.
The diff coverage is 75%.

@@                  Coverage Diff                   @@
##             gsoc-js-refactoring   #14470   +/-   ##
======================================================
  Coverage                       ?   50.53%           
  Complexity                     ?    14434           
======================================================
  Files                          ?      501           
  Lines                          ?    68012           
  Branches                       ?        0           
======================================================
  Hits                           ?    34372           
  Misses                         ?    33640           
  Partials                       ?        0

devenbansod

Looks good overall. Some comments.

Tested locally. Works fine.

devenbansod · 2018-08-02T15:35:57Z

webpack.config.babel.js

@@ -2,84 +2,95 @@ import path from 'path';
 import webpack from 'webpack';
 import BundleAnalyzerPlugin from 'webpack-bundle-analyzer';

-let BindleAnalyzer = BundleAnalyzerPlugin.BundleAnalyzerPlugin;
+function WebpackConfig (env) {
+    let BindleAnalyzer = BundleAnalyzerPlugin.BundleAnalyzerPlugin;


Typo? BundleAnalyzer?

devenbansod · 2018-08-02T15:36:50Z

webpack.config.babel.js

+    return {
+        mode: MODE,
+        entry: {
+            index_new: './js/src/index.js'


Still needs to be called as index_new?

Yeah as some of the files are still remaining.

devenbansod · 2018-08-02T15:38:48Z

scripts/build-js-files

+
+if [ -f package.json ] ; then
+    echo "Running Yarn Install"
+    yarn install


Does this assume that yarn is pre-installed, might be good to check it if possible?

Yeah, as I have gone through the script create-release, there isn't any check specified for composer so I guess there also we don't need to specify any check. Although, I tried adding the check but, didn't found anything about how to do that.
If you have any idea about how we can implement the check?

devenbansod · 2018-08-02T15:40:44Z

scripts/build-js-files

+
+#Performing cleanup
+#Removing yarn files
+rm yarn.lock


similar check as below would be good to have? -f yarn.lock?

devenbansod · 2018-08-02T15:47:29Z

scripts/build-js-files

@@ -0,0 +1,39 @@
+#!/bin/sh


Consider renaming this as a build-js.sh script?

devenbansod · 2018-08-02T15:52:00Z

scripts/build-js-files

+#Removing babel files as they are not required in production
+rm .babelrc
+rm webpack.config.babel.js
+


Unrelated to this file: Once the modularization is complete, we aim to delete the files in js/*.js right?

Yup but we need to rebase master over this branch first to take change from master to this branch which can also be added in the modular code.

devenbansod · 2018-08-02T15:55:59Z

scripts/build-js-files

@@ -0,0 +1,39 @@
+#!/bin/sh


Also, try to put the invocation for this script in its proper place inside scripts/create-release.sh?

Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>

Piyush3079

General comments to increase the understandability of the pr.

Piyush3079 · 2018-08-03T14:18:25Z

js/functions.js

@@ -5165,3 +5165,21 @@ function checkPasswordStrength (value, meter_obj, meter_object_label, username)
    case 4: meter_object_label.html(PMA_messages.strStrong);
    }
 }
+


Created a copy of function to make it globally available.

Piyush3079 · 2018-08-03T14:19:08Z

js/src/classes/Config.js

@@ -0,0 +1,80 @@
+import { PMA_Messages as PMA_messages } from '../variables/export_variables';


Validation class to validate input for settings tab

Piyush3079 · 2018-08-03T14:19:20Z

js/src/config.js

@@ -0,0 +1,176 @@
+/* vim: set expandtab sw=4 ts=4 sts=4: */


Modular code for config.js

Piyush3079 · 2018-08-03T14:20:03Z

js/src/functions/config.js

@@ -0,0 +1,589 @@
+import { PMA_ajaxShowMessage } from '../utils/show_ajax_messages';


General functions required to be imported in config.js for proper functionality.

Piyush3079 · 2018-08-03T14:21:17Z

js/src/variables/get_config.js

@@ -0,0 +1,33 @@
+import { validators } from '../classes/Config';


File containing functions for importing validation parameters in Modular code which are to be used in modular code.

Piyush3079 · 2018-08-03T14:21:57Z

libraries/classes/Config/FormDisplay.php

@@ -355,12 +355,12 @@ public function getDisplay(
            foreach ($this->_jsLangStrings as $strName => $strValue) {
                $jsLang[] = "'$strName': '" . Sanitize::jsFormat($strValue, false) . '\'';
            }
-            $js[] = "$.extend(PMA_messages, {\n\t"
-                . implode(",\n\t", $jsLang) . '})';
+            $js[] = "{\n\t"


Minor change as PMA_messages will not be available in global scope.

Piyush3079 · 2018-08-03T14:22:32Z

libraries/classes/Config/FormDisplay.php

        }

-        $js[] = "$.extend(defaultValues, {\n\t"
-            . implode(",\n\t", $jsDefault) . '})';
+        $js[] = "{\n\t"


Minor change as defaultValues will not be available in global scope

Piyush3079 · 2018-08-03T14:23:08Z

scripts/build-js.sh

@@ -0,0 +1,42 @@
+#!/bin/sh


Build script for creating production build of javascript files.

devenbansod

LGTM.

devenbansod changed the base branch from master to gsoc-js-refactoring August 1, 2018 11:18

Piyush3079 force-pushed the Mod_Js_Config_Js branch 10 times, most recently from 2da2163 to e251c8d Compare August 1, 2018 14:28

Add config related imports in files to use config.js in modular code.

5e04669

Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>

Piyush3079 force-pushed the Mod_Js_Config_Js branch from e251c8d to cae2c2b Compare August 1, 2018 14:32

Config.js is modularised.

3a754dc

Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>

devenbansod self-requested a review August 1, 2018 15:29

devenbansod self-assigned this Aug 1, 2018

Piyush3079 force-pushed the Mod_Js_Config_Js branch 3 times, most recently from e3a2240 to c1f42b0 Compare August 1, 2018 15:59

devenbansod reviewed Aug 2, 2018

View reviewed changes

Piyush3079 force-pushed the Mod_Js_Config_Js branch from c1f42b0 to 61e9780 Compare August 2, 2018 15:24

Piyush3079 added 3 commits August 2, 2018 20:54

Commented code removed and debug statements removed.

5463dab

Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>

Add isStorageSupport in functions.js for code compatibility and add c…

0b5c9b3

…onfig.js in file loading object. Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>

Pass env variable to webpack.config file for making production build.

61e9780

Build script for js files. Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>

Piyush3079 commented Aug 2, 2018

View reviewed changes

Piyush3079 force-pushed the Mod_Js_Config_Js branch from cfc9ff8 to 79577f8 Compare August 2, 2018 15:45

devenbansod reviewed Aug 2, 2018

View reviewed changes

Piyush3079 force-pushed the Mod_Js_Config_Js branch from 79577f8 to 03b8a7e Compare August 3, 2018 11:57

Piyush3079 force-pushed the Mod_Js_Config_Js branch 4 times, most recently from bffc1aa to 733b029 Compare August 3, 2018 12:34

Test fixtures for travis.

733b029

Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>

Piyush3079 commented Aug 3, 2018

View reviewed changes

devenbansod approved these changes Aug 7, 2018

View reviewed changes

devenbansod merged commit 0e16cc5 into phpmyadmin:gsoc-js-refactoring Aug 7, 2018

		@@ -2,84 +2,95 @@ import path from 'path';
		import webpack from 'webpack';

		@@ -0,0 +1,80 @@
		import { PMA_Messages as PMA_messages } from '../variables/export_variables';

		@@ -0,0 +1,589 @@
		import { PMA_ajaxShowMessage } from '../utils/show_ajax_messages';

		@@ -0,0 +1,33 @@
		import { validators } from '../classes/Config';

Config.js modularized #14470

Config.js modularized #14470

Conversation

Piyush3079 commented Jul 8, 2018

devenbansod commented Aug 1, 2018

devenbansod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piyush3079 Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piyush3079 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 2, 2018 • edited Loading

Codecov Report

devenbansod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piyush3079 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devenbansod left a comment

Choose a reason for hiding this comment

Piyush3079 Aug 2, 2018 •

edited

Loading

codecov bot commented Aug 2, 2018 •

edited

Loading