-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basic initial webpack configuration for Modular js code #14464
Basic initial webpack configuration for Modular js code #14464
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments/suggestions/requests for change.
.scrutinizer.yml
Outdated
@@ -24,4 +25,4 @@ build: | |||
command: './vendor/bin/phpcs --standard=PMAStandard ./ --report=checkstyle --report-file=cs-data --ignore=*/vendor/*,*/build/*' | |||
analysis: | |||
file: 'cs-data' # The reporter filename | |||
format: 'php-cs-checkstyle' # The supported format by Scrutinizer | |||
format: 'php-cs-checkstyle' # The supported format by Scrutinizer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/minor unneeded change
@@ -12,6 +12,7 @@ build: | |||
test: | |||
environment: | |||
php: 7.1 | |||
node: '8.11.3' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the exact number required? I'd prefer to specify the latest in 8.x.x if it can automatically pick it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure whether it will work or not as I have gone through the documentation and they have specified the exact version in examples.
https://scrutinizer-ci.com/docs/configuration/build#node
Also on nodejs.org this version is given stable
config.sample.inc.php
Outdated
* for making production build $cfg['environment'] = 'production' | ||
* | ||
*/ | ||
$cfg['environment'] = 'production'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should go to libraries/config.default.php
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have already been added in next pr.
I will add that webpack_host
directive here which needs to be used in development only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add them in this PR instead and remove it from the next PRs. We'll move to getting the next ones reviewed only when this one is ready and merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These settings only matter to developers, I think they should not be in this file, not even commented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.sample.inc.php
is intended to overwrite the default configurations in config.default.php
. So if developer wants to make change in webpack port and webpack host then he has to make change in config.default.php
. Is it the change required?
Also we need to make these directives available in webpack.config.babel.js
as there also, we need to specify some sort of configuration for port and output path on the basis of environment.
Creating a .env
file can solve webpack.config.babel.js
directive issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove it from the config.sample.inc.php
. It's meant for the users (not developers) who want minimum overhead of setting phpmyadmin up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I am removing it.
And adding a comment in config.default
$cfg['CSPAllow']
directive that it has to set to some value like http://localhost:3307 ws://localhost:3307
for webpack host to be localhost and webpack port to be 3307 so that it can allow loading of js files from cross-origin
js/ajax.js
Outdated
if (check[check.length - 1] === 'new.js') { | ||
var script_src = ''; | ||
if (PMA_commonParams.get('environment') === 'development') { | ||
script_src += 'http://localhost:' + PMA_commonParams.get('webpack_port') + '/js/dist/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should allow for specifying the webpack_host
too. I've personally seen and used hosts like localhost.xyz.com
while developing locally. Hardcoding anyway does not make sense, though the default value can be localhost
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think then there will not be the need of specifying the webpack port number as webpack_host will take care of that too.
So I will remove webpack_port and add webpack_host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hostname would just be example.com
(and would not have port). We'd still require the webpack_port
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it should be like hostname
:port
.
Ok I am updating it.
js/ajax.js
Outdated
if (PMA_commonParams.get('environment') === 'development') { | ||
script_src += 'http://localhost:' + PMA_commonParams.get('webpack_port') + '/js/dist/'; | ||
} else if (PMA_commonParams.get('environment') === 'production') { | ||
script_src = 'js/dist/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which one is better?
I see you have used +=
on L651, but use =
here. Is the initialization of script_src
to ''
really required?
js/src/db_search.js
Outdated
@@ -0,0 +1,251 @@ | |||
/* vim: set expandtab sw=4 ts=4 sts=4: */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this PR only focusses on Basic initial webpack configuration for Modular js code
, I wonder if this change (js/src/db_search.js) should go in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this files is not required in this pr but as this pr is made from gsoc branch, the commits or later branch will require this files so I thought of not removing this files as doing this will cause conflicts in later prs.
Also one of the purpose fulfilled by this files is testing of webpack configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can always rebase other PRs once this is merged. With regards to the testing, I believe we could do more exhaustive testing in the next PR too.
Add this here might make it trickier to come back to this PR later and wonder why was it needed. But I'm open to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this file. If it create some problem then I think I should remove it otherwise I just added it here to check if webpack is building the files correctly and the urls are working proporly.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a huge problem. But for any (potential) contributor, the PR looks cleaner w/o it. You can decide to remove it or keep it based on your judgement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you could remove it out, add it in a later PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I am removing this file from this pr
// Here would not be a good place to add CodeMirror because | ||
// the user preferences have not been merged at this point | ||
|
||
$this->_scripts->addFile('messages.php', array('l' => $GLOBALS['lang'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason for moving this code for addition of messages.php
to $this->_scripts
above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This files contains the message strings for js code so it should be loaded before any other other js files so that all the strings were available for any file being loaded.
libraries/classes/Scripts.php
Outdated
* It checks whether the file contains new in its name or not | ||
*/ | ||
if ($GLOBALS['cfg']['environment'] === 'development') { | ||
$src = "http://localhost:" . $GLOBALS['cfg']['webpack_port'] . "/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment regarding webpack_host
libraries/classes/Scripts.php
Outdated
*/ | ||
if ($GLOBALS['cfg']['environment'] === 'development') { | ||
$src = "http://localhost:" . $GLOBALS['cfg']['webpack_port'] . "/"; | ||
} else if ($GLOBALS['cfg']['environment'] === 'production'){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of else if
, we should initialize $src = ""
and have only if ($GLOBALS['cfg']['environment'] === 'development')
check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made required changes with some comments regarding some of the requested changes.
@@ -12,6 +12,7 @@ build: | |||
test: | |||
environment: | |||
php: 7.1 | |||
node: '8.11.3' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure whether it will work or not as I have gone through the documentation and they have specified the exact version in examples.
https://scrutinizer-ci.com/docs/configuration/build#node
Also on nodejs.org this version is given stable
config.sample.inc.php
Outdated
* for making production build $cfg['environment'] = 'production' | ||
* | ||
*/ | ||
$cfg['environment'] = 'production'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have already been added in next pr.
I will add that webpack_host
directive here which needs to be used in development only.
js/ajax.js
Outdated
if (check[check.length - 1] === 'new.js') { | ||
var script_src = ''; | ||
if (PMA_commonParams.get('environment') === 'development') { | ||
script_src += 'http://localhost:' + PMA_commonParams.get('webpack_port') + '/js/dist/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think then there will not be the need of specifying the webpack port number as webpack_host will take care of that too.
So I will remove webpack_port and add webpack_host.
js/src/db_search.js
Outdated
@@ -0,0 +1,251 @@ | |||
/* vim: set expandtab sw=4 ts=4 sts=4: */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this files is not required in this pr but as this pr is made from gsoc branch, the commits or later branch will require this files so I thought of not removing this files as doing this will cause conflicts in later prs.
Also one of the purpose fulfilled by this files is testing of webpack configuration.
// Here would not be a good place to add CodeMirror because | ||
// the user preferences have not been merged at this point | ||
|
||
$this->_scripts->addFile('messages.php', array('l' => $GLOBALS['lang'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This files contains the message strings for js code so it should be loaded before any other other js files so that all the strings were available for any file being loaded.
@Piyush3079 please look at the failures in Travis job at: https://travis-ci.org/phpmyadmin/phpmyadmin/jobs/401385243 and try to fix them. |
Codecov Report
@@ Coverage Diff @@
## gsoc-js-refactoring #14464 +/- ##
=========================================================
+ Coverage 50.43% 50.43% +<.01%
- Complexity 14428 14430 +2
=========================================================
Files 500 500
Lines 67846 67858 +12
=========================================================
+ Hits 34220 34227 +7
- Misses 33626 33631 +5 |
The failures were due to these tests stub the |
You might want to delay enforcing the strict |
I am working on this only. The |
.eslintrc.json
Outdated
}, | ||
// This comment to be removed only when working on local | ||
// It should not go on the PMA repo uncommented untill all the code gets cleaned. | ||
// "extends": "eslint:recommended", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original configuration is almost an extension of https://github.com/airbnb/javascript.
And if I remember correctly, JSON files doesn't allow comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"extends": "eslint:recommended",
this helps in enabling eslint in files strictly. If it is removed travis will not show any failure in tests for undefined functions and variables in files.
Although it is not needed now but we need to add this in future to make sure that there should not be any undefined variables as complete code is going to run in strict mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove it for now, and add it later then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that won't be problem I guess.
config.sample.inc.php
Outdated
* for making production build $cfg['environment'] = 'production' | ||
* | ||
*/ | ||
$cfg['environment'] = 'production'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These settings only matter to developers, I think they should not be in this file, not even commented.
libraries/config.default.php
Outdated
/** | ||
* Webppack port number for running development server of webpack | ||
*/ | ||
$cfg['webpack_port'] = 3307; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure if these are the most appropriate default values. $cfg['webpack_host']
and $cfg['webpack_port']
values are only interesting to developers, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah these configurations only need to be setup for developers. Normal users don't need these configurations.
package.json
Outdated
@@ -20,6 +24,14 @@ | |||
"zxcvbn": "4.4.2" | |||
}, | |||
"devDependencies": { | |||
"eslint": "^4.9.0" | |||
"babel-core": "^6.26.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should replace babel-core
by babel-cli
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I have done this in next pr but I will add in this pr also and remove from next one.
Also I am making change in production build script as it is creating some random directory like es6
. So please review this change also and suggest required changes.
@@ -0,0 +1,3 @@ | |||
{ | |||
"presets": ["env"] | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want, you can use the EditorConfig plugin to help with style issues like this and like most of the style issues pointed to by @phpmyadmin-bot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think this will be helpful. Thanks
package.json
Outdated
@@ -5,6 +5,12 @@ | |||
"repository": "https://github.com/phpmyadmin/phpmyadmin.git", | |||
"author": "The phpMyAdmin Team <developers@phpmyadmin.net> (https://www.phpmyadmin.net/team/)", | |||
"license": "GPL-2.0", | |||
"scripts": { | |||
"dev:wds": "webpack-dev-server --progress", | |||
"prod:build": "rimraf js/lib js/dist && babel js/src/ -d js/lib --ignore .test.js && cross-env NODE_ENV=production webpack -p --progress", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Piyush3079 Can we not use rm -rf
? (Is it required for Windows compatibility?)
It seems to have ISC license, which might require a review before adding it to our package.json (given that phpMyAdmin is GPL-v2 licensed).
cc: @ibennetch @nijel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, rm -rf
will give error to Windows users. That's why I have used this package to have cross-platform compatibility for creating build.
Looks good to me, except the few final comments. |
8032054
to
de8919a
Compare
/** | ||
* Working environment configuration | ||
* for development mode $cfg['environment'] = 'development', | ||
* for making production build $cfg['environment'] = 'production' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding a line here mentioning the need to also change CSPAllow, something like "You will probably also have to modify the $cfg['CSPAllow']
directive in order to use the webpack development server."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done that.
libraries/config.default.php
Outdated
@@ -3014,8 +3031,12 @@ | |||
|
|||
/** | |||
* Additional string to allow in CSP headers. | |||
* | |||
* For working envionment to be development, this has to be changed to allow | |||
* cross-origin loading of js files for specified webpack host and webpack port. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd spell out JavaScript instead of "js" here
libraries/config.default.php
Outdated
* | ||
* For working envionment to be development, this has to be changed to allow | ||
* cross-origin loading of js files for specified webpack host and webpack port. | ||
* Eg. 'http://localhost:3307 ws://localhost:3307' for locahost and 3307 webpack pot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"port" rather than "pot" :)
webpack.config.babel.js
Outdated
]; | ||
|
||
export default [{ | ||
// envionment either development or production |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "environment"
webpack.config.babel.js
Outdated
output: { | ||
filename: 'db_search_new.js', | ||
path: path.resolve(__dirname, 'dist'), | ||
publicPath: 'http://localhost:3307/js/dist' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work properly if the developer sets a different value for $cfg['webpack_host']
or cfg['webpack_port']
?
libraries/config.default.php
Outdated
@@ -3014,8 +3031,12 @@ | |||
|
|||
/** | |||
* Additional string to allow in CSP headers. | |||
* | |||
* For working envionment to be development, this has to be changed to allow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "environment"
doc/config.rst
Outdated
:type: string | ||
:default: `'production'` | ||
|
||
Enable you to set the working environment for loading js files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I suggest instead writing this?
Sets the working environment for loading JavaScript files for webpack development
Possible values are 'production' or 'development'
@@ -3402,6 +3402,29 @@ Developer | |||
|
|||
These settings might have huge effect on performance or security. | |||
|
|||
.. config:option:: $cfg['environment'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would $cfg['webpack_environment']
be more appropriate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making it webpack_environment
would restrict this directive to webpack only rather than making it for the complete application. With environmet
only we can use this directive for other things also like adding debugging in development environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think any other debugging functionality should have its own configuration directive (so developers can focus on debugging a specific module or functionality), but you have a valid point as well. It's not that important to change and besides, if a problem comes up later we can deal with it at that time.
If @MauricioFauth and @devenbansod are okay with this directive then it's fine for me as well.
doc/config.rst
Outdated
:default: `'http://localhost'` | ||
|
||
Enable you to start webpack development server on specified host for serving js files | ||
when working in the development environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just say:
Webpack development server hostname for serving JavaScript files. Requires
$cfg['environment']
to be development.
doc/config.rst
Outdated
:default: 3307 | ||
|
||
Enable you to start webpack development server on specified port for serving js files | ||
when working in the development environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly,
Webpack development server port for serving JavaScript files. Requires $cfg['environment'] to be development.
According to https://www.gnu.org/licenses/license-list.en.html#ISC, the ISC license is compatible with the GPL, so rifraf should be okay according to GNU. |
c7eece4
to
ec4c356
Compare
3008809
to
33002a3
Compare
…tive modules in package manager. Add script in package.json for building js files using webpack. Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>
Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>
…rectory, environment variable introduced Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>
and adding comments for some code handleing new js files. Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>
…es in default.config. Introduce webpack_host and webpack_port directive and updating package.json build scripts. Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>
33002a3
to
a6ec954
Compare
Hi @ibennetch @MauricioFauth, @Piyush3079 has done some of the suggested changes, rebased the PR and squashed the commits. How does the PR look to you? |
js/ajax.js
Outdated
if (PMA_commonParams.get('environment') === 'development') { | ||
script_src = PMA_commonParams.get('webpack_host') + ':' | ||
+ PMA_commonParams.get('webpack_port') + '/js/dist/'; | ||
} else if (PMA_commonParams.get('environment') === 'production') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think any value other than development
must be considered as production
.
…irectives are added in webpack.config Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>
a6ec954
to
4d3a8c9
Compare
This all looks great to me, thanks. |
Thanks everyone for the reviews. @Piyush3079 Great work on this! Thanks for your contribution. |
This pr contains the basic webpack configuration for modular js code with some basic changes in Header and Script class and the javascript AJAX handler for handling the new js code files.
Before submitting pull request, please check that every commit: