Skip to content

Commit b5423dd

Browse files
committed
Adding docs and cleanup
1 parent 09b833a commit b5423dd

File tree

13 files changed

+5097
-171
lines changed

13 files changed

+5097
-171
lines changed
File renamed without changes.
File renamed without changes.

guides/rules/no-action-cp.md

Whitespace-only changes.

guides/rules/no-attrs.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
# No Attrs
2+
3+
**TL;DR** Use `_` to prefix things that are local and leave passed items as the bare name.
4+
5+
In the runup to Ember 2.0, several blog articles were written about using `this.attrs` but they have never been documented as a public API. Typically people use attrs to denote things as "passed" and bare names as local. This is useful and some iteration of Ember will have this built into the programming model, but for now we should not use `attrs`.
6+
7+
In JavaScript we typically prefix "private" things in JavaScript with `_`. If you want to create this notion in a component, we can leverage this long standing convention. Things that are local are technically private as component's scope is isolated so marking them with `_` semantically makes sense. Passed items can use the bare name, as they are effectively public/protected methods and properties.

guides/rules/no-observers.md

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
# No Observers
2+
3+
**TL;DR: Use a computed property or lifecycle hooks**
4+
5+
Observers suffer from several issues when building applications at scale. These issues are:
6+
7+
- Eagerly computed
8+
- Have no notion of a caller
9+
- Data consistency
10+
11+
To unroll what this means consider the example below.
12+
13+
```
14+
Compontent.extend({
15+
firstName: 'Chad',
16+
lastName: 'Hietala',
17+
fullName: undefined,
18+
fullDidChange: observer('firstName', 'lastName', function() {
19+
this.set('fullName',`${this.get('firstName')} ${this.get('lastName')}`);
20+
})
21+
});
22+
```
23+
24+
In the above example we have an observer that is watching the `firstName` and `lastName` properties in our component. When either of them change the observer will fire, setting `fullName` to the new value. Vast majority of the time this leads to `fullName` being computed more often than it needs to be. There is also an unexpected eventual consistency problem here. Consider we do the following.
25+
26+
```
27+
this.set('firstName', 'Stefan');
28+
<---------------------------------------- For a period here `fullName` is Stefan Hietala
29+
this.set('lastName', 'Penner');
30+
<---------------------------------------- Now `fullName` is Stefan Penner
31+
```
32+
33+
Sense observers eagerly compute we have this period in time where we have computed a `fullName` that is not what actually care about. When we introduce many observers into an application this problem compounds on it's self.
34+
35+
In contrast a computed property is lazily computed and memoized. This means that `firstName` and `lastName` can freely mutate and only when we do `this.get('fullName');` will the function body of the computed property will run. This gives us a clear caller in terms of why the body of why `fullName` was recomputed.
36+
37+
```
38+
Compontent.extend({
39+
firstName: 'Chad',
40+
lastName: 'Hietala',
41+
fullName: computed('firstName', 'lastName', function() {
42+
return `${this.get('firstName')} ${this.get('lastName')}`;
43+
})
44+
});
45+
```
46+
47+
Since computed properties are lazy we don't have the same data consistency problems with observers.
48+
49+
```
50+
this.get('fullName'); <------------------ Computed property is computed to `Chad Hietala`
51+
this.set('firstName', 'Stefan'); <------- Computed property does not recompute
52+
this.set('lastName', 'Penner'); <-------- Computed property does not recompute
53+
this.get('fullName'); <------------------ Computed property is computed to `Stefan Penner`
54+
```
55+
56+
For more info please watch this video: [Observer Tip Jar](https://www.youtube.com/watch?v=vvZEddrClAQ)

lib/index.js

Lines changed: 17 additions & 171 deletions
Original file line numberDiff line numberDiff line change
@@ -1,172 +1,18 @@
1-
const FrameworkObjects = ['Component', 'Route', 'Service', 'Helper'];
2-
const FrameworkHooks = ['init', 'didReceiveAttrs'];
3-
4-
export default function(context) {
5-
let services = [];
6-
let errors = [];
7-
let members = [];
8-
9-
function isUsingAttrsSnapShots(node) {
10-
if (node.key.name === 'didReceiveAttrs' && node.value.params.length > 0) {
11-
node.value.params.forEach((param) => {
12-
errors.push([param, 'Do not use the previous and new versions of attrs. If you need to do a comparsion on re-render stash the last value and compare the new value.']);
13-
})
14-
}
15-
}
16-
17-
function isCPMissingDependentKey(node) {
18-
if (isCP(node)) {
19-
if (node.value.arguments.length === 1 && node.value.arguments[0].type === 'FunctionExpression') {
20-
errors.push([node.value.arguments[0], 'Missing dependent key. If you are working around a value being shared acrossed instances use `init`']);
21-
}
22-
}
23-
}
24-
25-
function isCP(node) {
26-
return node.value.type === 'CallExpression' && node.value.callee.property.name === 'computed';
27-
}
28-
29-
function isAttrs(node) {
30-
if (node.property.name === 'attrs' && node.object.type === 'ThisExpression') {
31-
errors.push([node.property, 'Do not use this.attrs.']);
32-
}
33-
}
34-
35-
function isFrameWorkObject(node) {
36-
return node.object.type === 'Identifier' && FrameworkObjects.indexOf(node.object.name) === -1;
37-
}
38-
39-
function isExtendingFrameworkObject(node) {
40-
if (isExtend(node) && isFrameWorkObject(node)) {
41-
errors.push([node, 'Do not create subclasses. Please use more compositional patterns described here.']);
42-
}
43-
}
44-
45-
function isExtend(node) {
46-
return node.property.name === 'extend';
47-
}
48-
49-
function isGetMethod(node) {
50-
return node.property.name === 'get' && node.object.type === 'ThisExpression';
51-
}
52-
53-
function isService(node) {
54-
return services.indexOf(node.value) > -1
55-
}
56-
57-
function isFrameworkHook() {
58-
return FrameworkHooks.indexOf(context.getScope().block.parent.key.name) > -1;
59-
}
60-
61-
function isEagerLazyInjection(node) {
62-
if (isGetMethod(node) && isService(node.parent.arguments[0]) && isFrameworkHook()) {
63-
errors.push([node.parent.arguments[0], 'Do not pull on lazy injections in framework hooks. If you need a service eagerly please inject it through an initializer.']);
64-
}
65-
}
66-
67-
function isMixin(node) {
68-
if (isExtend(node)) {
69-
if (node.parent.arguments.length > 1) {
70-
node.parent.arguments.filter((arg) => {
71-
return arg.type === 'Identifier'
72-
}).forEach((arg) => {
73-
errors.push([arg, 'Do not use Mixins. Please use more compositional patterns described here.']);
74-
});
75-
}
76-
}
77-
}
78-
79-
function collectInjectedProperties(node) {
80-
if (node.value.type === 'CallExpression' && node.value.callee.property.name === 'service') {
81-
services.push(node.key.name);
82-
}
83-
}
84-
85-
function isInjectedService(node) {
86-
return node.value.callee && node.value.callee.property.name === 'service';
87-
}
88-
89-
function emptyArgs(node) {
90-
return node.arguments.length === 0;
91-
}
92-
93-
function isMissingInjectionName(node) {
94-
if (isInjectedService(node) && emptyArgs(node.value)) {
95-
errors.push([node.value.callee.property, 'Provide the name as a service to avoid reflection on the property name.'])
96-
}
97-
}
98-
99-
function isSettingInCPGetter(node) {
100-
if (node.parent) {
101-
// console.log(node.parent.parent.parent)
102-
}
103-
}
104-
105-
function isSendAction(node) {
106-
107-
}
108-
109-
function findMethod() {
110-
let scope = context.getScope().block;
111-
let top = true;
112-
let methodName;
113-
114-
while (top) {
115-
if (scope.parent.type === 'Property') {
116-
if (scope.parent.parent.parent.type === 'CallExpression' && scope.parent.parent.parent.callee.property.name === 'extend') {
117-
methodName = scope.parent.key.name;
118-
top = false;
119-
}
120-
} else {
121-
scope = scope.parent;
122-
}
123-
}
124-
125-
return methodName
126-
}
127-
128-
function isSideEffect(node) {
129-
if (node.parent.callee && node.object.type === 'ThisExpression' && members.indexOf(node.property.name) === -1) {
130-
let currentMethod = findMethod();
131-
132-
if (FrameworkHooks.indexOf(currentMethod) > -1) {
133-
console.log(node.property.name)
134-
}
135-
}
136-
}
137-
138-
function collectMembers(node) {
139-
if (node.value.type === 'FunctionExpression') {
140-
members.push(node.key.name);
141-
}
142-
}
143-
144-
return {
145-
MemberExpression(node) {
146-
isAttrs(node);
147-
isExtendingFrameworkObject(node);
148-
isMixin(node);
149-
isEagerLazyInjection(node);
150-
isSendAction(node);
151-
//isSideEffect(node);
152-
isSettingInCPGetter(node)
153-
154-
},
155-
156-
Property(node) {
157-
collectMembers(node);
158-
collectInjectedProperties(node);
159-
isCPMissingDependentKey(node);
160-
isUsingAttrsSnapShots(node);
161-
isMissingInjectionName(node);
162-
163-
},
164-
165-
'Program:exit'() {
166-
errors.forEach((err) => {
167-
context.report(err[0], err[1]);
168-
});
169-
},
170-
};
1+
module.exports = {
2+
rules: [
3+
'./rules/no-observers',
4+
'./rules/no-attrs',
5+
'./rules/no-action-cp',
6+
'./rules/no-component-unit-tests',
7+
'./rules/no-mixins',
8+
'./rules/no-subclassing',
9+
'./rules/no-attrs-snapshots',
10+
'./rules/no-set-interval',
11+
'./rules/no-set-timeout',
12+
'./rules/detect-stateless-service',
13+
'./rules/eager-injections',
14+
'./rules/cp-send-action',
15+
'./rules/missing-dependant-keys',
16+
'./rules/enforce-fastboot'
17+
]
17118
};
172-

lib/rules/no-action-cp.js

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
import { get } from '../utils/get';
2+
import { getCaller } from '../utils/caller';
3+
4+
5+
function isCPDesc(node, method) {
6+
if (node.type !== "FunctionExpression") {
7+
return false;
8+
}
9+
10+
if (get(node, "parent.key.name") !== method) {
11+
return false;
12+
}
13+
14+
let grandParent = get(node, "parent.parent");
15+
if (!grandParent) {
16+
return false;
17+
}
18+
if (grandParent.type !== "ObjectExpression") {
19+
return false;
20+
}
21+
var greatGrandParent = pParent.parent; // more parents!
22+
if (greatGrandParent.type !== "CallExpression") {
23+
return false;
24+
}
25+
var callee = greatGrandParent.callee;
26+
if (greatGrandParent.type === "Identifier" && callee.name === "computed") {
27+
return true;
28+
}
29+
if (callee.type === "MemberExpression") {
30+
var caller = getCaller(callee);
31+
return caller === "Ember.computed" || caller === "Em.computed";
32+
}
33+
return false; // don't know how you could get here
34+
}
35+
36+
function _isCpGetter2(node) {
37+
if (node.type !== "FunctionExpression") {
38+
return false;
39+
}
40+
var parent = node.parent;
41+
if (parent.type !== "CallExpression") {
42+
return false;
43+
}
44+
var callee = parent.callee;
45+
if (callee.type === "Identifier" && callee.name === "computed") {
46+
return true;
47+
}
48+
if (callee.type === "MemberExpression") {
49+
var caller = n.getCaller(callee);
50+
return caller === "Ember.computed" || caller === "Em.computed";
51+
}
52+
return false;
53+
}
54+
55+
function isCpGetter(node) {
56+
return isCPDesc(node, 'get') || isCPAccessor(node)
57+
}
58+
59+
60+
export default function noActionCp(context) {
61+
62+
}

lib/rules/no-attrs.js

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
function isAttrs(node) {
2+
return node.property.name === 'attrs' && node.object.type === 'ThisExpression';
3+
}
4+
5+
export default function noAttrs(context) {
6+
return {
7+
MemberExpression(node) {
8+
if (isAttrs(node)) {
9+
context.report(node.property, `Do not use this.attrs. Please see the following guide for more infromation: http://github.com/chadhietala/ember-best-practices/files/guides/no-observers.md`);
10+
}
11+
}
12+
}
13+
}

lib/rules/no-observers.js

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
function isObserver(node) {
2+
let { property, callee } = node;
3+
return property && property.name === 'observer' ||
4+
callee && callee.name === 'obsever';
5+
}
6+
7+
const MESSAGE = `Do not use observers. Consider using computed properties instead. Please see following guide for more information: http://github.com/chadhietala/ember-best-practices/files/guides/no-observers.md`;
8+
9+
export default function noObservers(context) {
10+
return {
11+
MemberExpression(node) {
12+
if (isObserver(node)) {
13+
context.report(node.property, MESSAGE);
14+
}
15+
},
16+
17+
CallExpression(node) {
18+
if (isObserver(node)) {
19+
context.report(node.callee, MESSAGE);
20+
}
21+
}
22+
};
23+
};

0 commit comments

Comments
 (0)