-
Notifications
You must be signed in to change notification settings - Fork 14
Home
Liferay has both a rigorous code review process, and strict rules about how code should be formatted, which was why this tool was created in the first place. This document seeks to communicate the main principles behind the conventions and explain the reasoning for the different source code conventions and why you may have gotten a particular error.
There are a few core principles that lie underneath all of the conventions, as arbitrary as they may seem at times. When developers don't understand the reasoning for certain rules, they are less likely to absorb why those rules are important and less likely to actually apply them, or look for areas where things could be improved to fit the principles. As a company, the Liferay culture values these things in it's code:
Consistency is the foundation of trust. Code that is different only to be different causes confusion, and wastes time by forcing other developers to think through which pattern they should apply. If things are consistent, even if they're not ideal, they're easier to fix and improve. As styles fall in and out of favor, and coding fads come and go, there is a temptation for different developers to code in a certain way. How do you judge if it's worth changing a pattern? If it's worth changing, it's worth changing everywhere. This keeps us from introducing superficial changes that break convention, and helps ensure the changes we do make are worthwhile.
Code is read far more often than it's written. Our code should be easy enough to understand without comments. Naming should be taken with care, thinking through how other developers will understand the meaning of the code. Logic should be explicit but concise, and "magic" and "clever" code should be avoided. Whitespace is your friend, use it smartly to group your code and make it easy to visually scan. Where possible, sort things alphabetically.
There are times to break specific conventions, and it's all about the context. Languages are different and what is ideal in one is not the same as another. Code makes a good servant but a terrible master, and ultimately it is here to serve a higher purpose. So in certain situations, such as extremely performance critical contexts or avoiding negative side effects to the user, we should do what is best to accomplish the larger goal.
There should be no more than 1 empty line between two lines of code.
This error indicates that there are invisible whitespace characters in your code. These characters never have any useful purpose and often are inserted by some editors.
In Liferay, we use tabs for indentation. This error means you have a space in your indentation.
All CSS properties and @includes should be alphabetized.
The format for the letters in hex colors should always be uppercase. So #FFF is correct, but not #0cc.
This error means that your hex code can be shortened to 3 characters instead of 6. This is because in CSS, hex colors are actually 3 units of 2 characters, such as #FF 00 CC, and if the 2 characters in the group are identical, they can be shortened to #F0C.
Numbers that have decimal places should have a leading number. For instance, margin: .1em should instead be margin: 0.1em.
The reason is to make it obvious that it contains a decimal place, which could be easy to miss when visually scanning the code.
Selector blocks should have a single newline between them, and nested selectors should have a single newline between the last property and the first selector.
.foo {
}
.bar {
padding: 5px;
.baz {
margin: 5px;
}
}
Comma-separated lists, such as those in rules like clip: rect(0,0,0,0) should have a single space after each comma.
Selectors should be have a single space beween the rule and the opening bracket, like so: .foo {
In CSS, you don't need to quote values passed into url() calls.
Whether it's 0px, 0em, or 0%, it is still the same value: 0.
This indicates that there is an extraneous newline in the code (perhaps before the close of a rule, or more than one newline together). The most number of lines anything is separated by is 1.
This indicates that your rule has a trailing comma in the selector, like .foo, {.
This indicates that your rule has a border property like border: none or border: 0. We discourage this because it forces anyone who wishes to add the border back, to also have to duplicate the original color as well.
For instance, doing this:
// Default rule
.rule {
border: 1px solid #900;
}
//... some other file
.rule {
border: none;
}
// User's theme
.rule {
border: 1px solid #900;
}
Which as you can see means they have to keep their color and border style in sync and duplicate whatever the default is.
What's instead better is if you wish to remove a border, do border-width: 0;, which has the same effect, but allows someone to add the border back and not require them to duplicate the defaults.
This indicates that your property is formatted as something like either color:#FFF or color: #FFF.
There should be exactly one space after the property (and no spaces before).
When someone uses <div> without any sort of identifying information, it's usually a code smell that they are trying to use the div for it's side-effects (that it's a block element, and will place it's contents onto it's own line).
However, this is not good for semantic reasons, as well as the fact that it limits theme developers from being able to style it to behave differently.
Add either a class or an ID to this element to both make it's purpose clear and give developers a hook into styling it.
Why only block level tags? Well, technically it's not all block level items. Items like <p> paragraph tags are allowed to be anonymous, since it's they're not usually targetable (and if they are, they're targetable pretty predictably, or will be flagged with a class or ID).
It's usually just div tags that are sprinkled haphazardly, but it's common to see something like:
<ul class="list">
<li>Foo</li>
<li>Bar</li>
</ul>Or a semantically neutral:
<div class="generic-list">
<span>Foo</span>
<span>Bar</span>
</div>But if you find that you have random text blocks dynamically placed in certain areas, you can go with either a span or div, but in either case, it's better to give it a class name, just in case.
Attributes on elements should be sorted alphabetically.
Values inside of many attributes should be sorted alphabetically. For instance, the names of CSS classes inside of the class attribute, or the CSS rules inside of a style attribute.
We try to ignore most attributes where this doesn't make sense, but the general rule is, if there is a list of values in the attribute, it should probably be sorted.
The general rule here is that if any of the arguments to a function are on their own line, then all of the arguments should be on their own line. If the function has no arguments, it should be on one line, and if none of the arguments break to a new line, they should all be on the same line. If any of the arguments are an anonymous function, or an object with properties, all of the arguments should be on their own line. For example:
alert(1,
2,
3);
alert(function(){
},
1, 2);
alert(
1, 2, 3
);
alert(
);
alert({x: 1});
alert({}, 1);
alert(
function() {
}
);
alert(1, 2, 3);
Whenever you have a closing parenthesis or keyword followed by an opening bracket (like so: ) {, or try {), there should always be exactly 1 space between them.
This includes if, while, for, try, etc.
Anonymous functions, and function declarations should always be formatted as function() { with no spaces after the function keyword.
Liferay.Language.get() will do a synchronous AJAX request to the server to retrieve the proper string value for the language key. This is to guarantee correctness, but has many downsides, mainly being that it will lock up the browser until the value has returned. However, we also pre-process .js files so that the strings are replaced inline with the proper localized string for the users language, and thereby avoiding the synchronous AJAX calls (or the call to Liferay.Language.get() at all).
When a variable is passed to Liferay.Language.get(), the server has no way to resolve the JS variable (since it will be at execution time of that code block), and can't replace the value, in effect forcing a synchronous call.
For the same reasons as the previous rule, since we don't process non JS files for Liferay.Language.get(), this will also trigger a synchronous AJAX call.
This means you have either a console.log(), console.trace(), console.warn(), etc. within your code (basically, any method on the console object is assumed to be a debugging statement).
We distinguish between variable declaration/assignment and code which is acting on those variables. eg.
var foo = function(){
};
var bar = 0;
setTimeout(foo, bar);
var foo = function(){
};
var bar = 0;
setTimeout(foo, bar);
The cause of this is because the purpose of Liferay.provide is to provide a function that will be available synchronously but that will execute asynchronously.
The purpose of the use="..." attribute is to load execute the body of the script tag asynchronously.
By defining a function inside of an asynchronous <aui:script> means that the function could be defined at any time, including after when the function is called, which will throw an error. This also means that the error could appear sporadically, making for hard to debug issues.
Here's the possible error flow:
- Async request is made for the JS
- Network delay...
- User clicks on button that tries to execute
foo() - Error thrown because foo is defined in the async callback
Arrays should not contain spaces at the beginning or end of the array, e.g.
[ 1, 2, 3 ]
[ 1, 2, 3]
[1, 2, 3 ]
[1, 2, 3]
Arrays should have exactly one space between elements
[1,2,3]
[1, 2, 3]
[1, 2, 3]
Catch statements are passed an error argument, and that argument name should be e.
Empty catch statements should be formatted with the closing bracket exactly 1 line after the opening, e.g.:
catch (e) {
}
Constants should be separated with exactly one line between them, e.g.
var BAR = 1;
var FOO = 2;
Variables which span over multiple lines (for instance, strings of multiline HTML), should be formatted so that the start of the last line is aligned to the start of the variable name, not the var keyword, e.g.
var FOO = '<p>' +
'<span></span>'
'</p>';
var FOO =
'<p>' +
'<span></span>'
'</p>';
var FOO = '<p>' +
'<span></span>'
'</p>';
Liferay.provide() has a very specific method signature, and every argument is required.
This error usually occurs when someone doesn't understand the purpose of the method.
It is not a way to create a general function, or a method on a property, and it would not be ideal to do it with Liferay.provide().
The purpose of Liferay.provide() is to declare a function which depends on a set of modules, and only once those modules are loaded, will it execute.
The signature of the method is:
Liferay.provide(
object (to add the method to),
string (name of the method),
function (to execute once the dependencies are loaded),
array (of dependencies)
);
Variables should not be declared with a single var statement, but each one should have it's own.
The reason for this is because when assigning variables like so:
var foo = 1,
bar = 2,
baz = 3;
It can become very easy to not see a missing comma, leading to the creation of a global variable.
Having the var statement also helps break up visually the area for declaring variables vs. the area for working with them.
You only need one semicolon to terminate a line. Anything more than that is not necessary.
The reason this is logged as an error is because the common convention for getter methods that return a boolean property is to use isFoo(), which would return the state of foo. However, if the property is named .isFoo, the by convention, the getter method would be .isIsFoo, which is odd.
Instead, it's easier to just have .foo and .isFoo().
Undefined variables are either references to implied global objects or going to cause an explicit error.
If the variable is an implied global, add this line to the top of your JS file (assuming the name of the variable is SomeGlobalVar): /* global SomeGlobalVar */ which will remove the error.
Constants should be sorted alphabetically, whenever possible. If the constant is needed to be defined because it's used in another constant later, then it's okay, and we won't flag that as an error. However, all others should be sorted.
All properties and methods on an object should be sorted alphabetically amongst their type, and the types should be ordered as:
var obj = {
// Public properties
// Public methods
// Private methods
// Private properties
};
Inside of a list of a YUI modules dependencies (which are stored in a requires array), the names of the modules should be alphabetically sorted.
TODO ESLint Rule Documentation
TODO ESLint Rule Documentation
TODO ESLint Rule Documentation
TODO ESLint Rule Documentation
TODO ESLint Rule Documentation