[#19717] - [4.0] Airbnb ECMAScript 2017 (ES6) Coding Standards for the files in directory build/build-modules-js
- Closed
- 24 Jun 2018
- Medium
- Build: 4.0-dev
- # 19717
- Diff
- astridx:j4_airbnb/es6/codingstandards
User tests: Successful: Unsuccessful:
This PR is part of a javascript modernization plan. See #19643 for more details.
Summary of Changes
I edited the 6 js-files in directory /build/build-modules-js/ so that this files fullfil the coding standards.
I added three dependencies to the devDependencies of package.json, because we use it and coding standards require this. If I am right, we only use it for build process:
https://github.com/postcss/postcss
https://github.com/petkaantonov/bluebird
https://github.com/chalk/chalk
I added one dependency to the dependencies of package.json, because we use it and coding standards require this. If I am right we use it in atum template.
https://github.com/claviska/jquery-minicolors
I changed the file .eslintignore analogous to this PR: https://github.com/joomla/joomla-cms/pull/19650/files#diff-1dc6ee56b778cd91e0327b52aaeaa1b9R16
Testing Instructions
- Apply this patch
- Run
npm install - Make sure that the coding standards throw no error:
Run
node ./node_modules/eslint/bin/eslint.js build/build-modules-js/*
If you want to check only one file run:
node ./node_modules/eslint/bin/eslint.js build/build-modules-js/compilecejs.js
node ./node_modules/eslint/bin/eslint.js build/build-modules-js/compilecescss.js
node ./node_modules/eslint/bin/eslint.js build/build-modules-js/compilejs.js
node ./node_modules/eslint/bin/eslint.js build/build-modules-js/compilescss.js
node ./node_modules/eslint/bin/eslint.js build/build-modules-js/installation.js
node ./node_modules/eslint/bin/eslint.js build/build-modules-js/update.js
- Make sure, that the script do still that what they should do
Run the commands
node build –compilecejs
node build –compilececss
node build –compilejs
node build –compilescss
node build –installation
node build –update
--update (Updates the vendor scripts)
--compilejs, --compilejs path (Compiles ES6 to ES5 scripts)
--compilecss, --compilecss path (Compiles all the scss files to css)
--compilecejs, --compilecejs path (Compiles/transpiles all the custom elements files)
--compilecescss, --compilecescss path (Compiles/transpiles all the custom elements files
--installer (Creates the language file for installer error page)
Expected result
Everything works fine and there are no coding standard errors.
Actual result
Everything works fine, but there are coding standard errors.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | JavaScript Repository |
| Labels |
Added:
?
|
||
First of all thank you @astridx this is great!
Just one small observation here, since the project has some devs that use winDOS machines I think all the paths may need to use Path.join(rootPath, '/media/system/webcomponents/css') instead of ${rootPath}/media/system/webcomponents/css but since I don't have such a machine I cannot test the code to check is either ways are working
@dgt41 Thanks for your feedback. I tested this with Ubuntu. Currently I do not have a Windows machine, too. Why do you think, that we can not use template strings? In a quick search I found no website, that describes this problem. But I found this: Template strings are possible with Edge: https://docs.microsoft.com/en-us/scripting/javascript/advanced/template-strings-javascript
Tested this on Windows:
33 warnings
warning Unexpected console statement no-console
warning Unexpected unnamed function func-names
You can remove the first warning by adding "no-console": "off" to .eslintrc
At the first try, I have only eliminated the errors. Now I have also eliminated the warnings.
I added "no-console": "off" to .eslintrc and named the unnamed files.
Imo we should not disable the console rule. We should not have console statements in production code (i found some occurences in the production browser scripts). Most of the time (>99% in browser environment) this is debug code that sb. has forgotten to remove. Even in node environment (/build/ folder) we can replace it with a simple logger and/or disable the rule for special cases.
Its ok to merge this when all cs errors are resolved. We can resolve the warnings, when we refactor the build scripts and create a simple logger.
Even in node environment (/build/ folder) we can replace it with a simple logger
True but on screen logging is better in this case
Yes, but this does not mean that we cannot use a simple log helper. For me it is more important to find the unwanted usages in our browser scripts like https://github.com/joomla/joomla-cms/blob/4.0-dev/media/com_menus/js/admin-menus-default.js#L14.
So i would re-enable the console rule and merge this PR.
The warnings are displayed again and the conflict is solved.
I'm closing this PR. He is already very old and contains many conflicts.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-06-24 12:02:20 |
| Closed_By | ⇒ | astridx |
LGTM, thank you astrid!
When this is merged, we can expand hound ci to auto-check the codestyles.
ping @dgt41