Skip to content

Commit 2f25651

Browse files
committed
Move flash errors to service
Dependencies between controllers should be avoided and moved to services instead. This is because the lifecycle of controllers is slightly unpredictable, as its tied to routes and if for example a controller is used by multiple routes, you might get funky behaviour. With services, the expectation is different, so one tends to use it according to said expectations. For the flash messages services I thought I would encapsulate the behaviour and expose what I hope are meaningful methods, `show` for immediately displaying the error, `queue` for displaying it on the next transition, and `step` to actually cycle the errors on transition. I also abstracted the rendering of the flash error message into a component, injecting the flash messages service to display the error and control visibility.
1 parent c13110a commit 2f25651

File tree

17 files changed

+69
-28
lines changed

17 files changed

+69
-28
lines changed

app/components/flash-message.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import Ember from 'ember';
2+
3+
export default Ember.Component.extend({
4+
flashMessages: Ember.inject.service(),
5+
message: Ember.computed.readOnly('flashMessages.message'),
6+
7+
elementId: 'flash',
8+
tagName: 'p',
9+
classNameBindings: ['message:shown']
10+
});

app/controllers/application.js

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,6 @@ import Ember from 'ember';
22

33
export default Ember.Controller.extend({
44
searchController: Ember.inject.controller('search'),
5-
6-
flashError: null,
7-
nextFlashError: null,
85
search: Ember.computed.oneWay('searchController.q'),
96

107
init() {
@@ -54,11 +51,6 @@ export default Ember.Controller.extend({
5451
Ember.$(document).off('keydown');
5552
},
5653

57-
stepFlash() {
58-
this.set('flashError', this.get('nextFlashError'));
59-
this.set('nextFlashError', null);
60-
},
61-
6254
actions: {
6355
search() {
6456
this.transitionToRoute('search', {

app/controllers/me/index.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import Ember from 'ember';
22
import ajax from 'ic-ajax';
33

44
export default Ember.Controller.extend({
5+
flashMessages: Ember.inject.service(),
6+
57
isResetting: false,
68

79
actions: {
@@ -21,7 +23,7 @@ export default Ember.Controller.extend({
2123
} else {
2224
msg = 'An unknown error occurred';
2325
}
24-
this.controllerFor('application').set('nextFlashError', msg);
26+
this.get('flashMessages').queue(msg);
2527
// TODO: this should be an action, the route state machine
2628
// should recieve signals not external transitions
2729
this.transitionToRoute('index');

app/mixins/authenticated-route.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import Ember from 'ember';
22

33
export default Ember.Mixin.create({
4+
flashMessages: Ember.inject.service(),
5+
46
beforeModel(transition) {
57
let user = this.session.get('currentUser');
68
if (user !== null) {
@@ -19,8 +21,7 @@ export default Ember.Mixin.create({
1921
});
2022
} else {
2123
this.session.set('savedTransition', transition);
22-
this.controllerFor('application')
23-
.set('nextFlashError', 'Please log in to proceed');
24+
this.get('flashMessages').queue('Please log in to proceed');
2425
return this.transitionTo('index');
2526
}
2627
},

app/routes/application.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import Ember from 'ember';
22
import ajax from 'ic-ajax';
33

44
export default Ember.Route.extend({
5+
flashMessages: Ember.inject.service(),
6+
57
beforeModel() {
68
if (this.session.get('isLoggedIn') &&
79
this.session.get('currentUser') === null) {
@@ -20,7 +22,7 @@ export default Ember.Route.extend({
2022

2123
actions: {
2224
didTransition() {
23-
this.controllerFor('application').stepFlash();
25+
this.get('flashMessages').step();
2426
},
2527
},
2628
});

app/routes/category.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
import Ember from 'ember';
22

33
export default Ember.Route.extend({
4+
flashMessages: Ember.inject.service(),
5+
46
model(params) {
57
return this.store.find('category', params.category_id).catch(e => {
68
if (e.errors.any(e => e.detail === 'Not Found')) {
7-
this.controllerFor('application').set('flashError', `Category '${params.category_id}' does not exist`);
9+
this.get('flashMessages').show(`Category '${params.category_id}' does not exist`);
810
}
911
});
1012
}

app/routes/crate.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
import Ember from 'ember';
22

33
export default Ember.Route.extend({
4+
flashMessages: Ember.inject.service(),
5+
46
model(params) {
57
return this.store.find('crate', params.crate_id).catch(e => {
68
if (e.errors.any(e => e.detail === 'Not Found')) {
7-
return this.controllerFor('application').set('flashError', `Crate '${params.crate_id}' does not exist`);
9+
this.get('flashMessages').show(`Crate '${params.crate_id}' does not exist`);
10+
return;
811
}
912
});
1013
},

app/routes/crate/docs.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import Ember from 'ember';
22

33
export default Ember.Route.extend({
4+
flashMessages: Ember.inject.service(),
5+
46
redirect() {
57
let crate = this.modelFor('crate');
68

@@ -11,7 +13,7 @@ export default Ember.Route.extend({
1113
// Redirect to the crate's main page and show a flash error if
1214
// no documentation is found
1315
let message = 'Crate does not supply a documentation URL';
14-
this.controllerFor('application').set('nextFlashError', message);
16+
this.get('flashMessages').queue(message);
1517
this.replaceWith('crate', crate);
1618
}
1719
},

app/routes/crate/repo.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import Ember from 'ember';
22

33
export default Ember.Route.extend({
4+
flashMessages: Ember.inject.service(),
5+
46
redirect() {
57
let crate = this.modelFor('crate');
68

@@ -11,7 +13,7 @@ export default Ember.Route.extend({
1113
// Redirect to the crate's main page and show a flash error if
1214
// no repository is found
1315
let message = 'Crate does not supply a repository URL';
14-
this.controllerFor('application').set('nextFlashError', message);
16+
this.get('flashMessages').queue(message);
1517
this.replaceWith('crate', crate);
1618
}
1719
},

app/routes/crate/version.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import Ember from 'ember';
22
import ajax from 'ic-ajax';
33

44
export default Ember.Route.extend({
5+
flashMessages: Ember.inject.service(),
6+
57
refreshAfterLogin: Ember.observer('session.isLoggedIn', function() {
68
this.refresh();
79
}),
@@ -89,7 +91,7 @@ export default Ember.Route.extend({
8991
.then(versions => {
9092
const version = versions.find(version => version.get('num') === params.version_num);
9193
if (params.version_num && !version) {
92-
this.controllerFor('application').set('nextFlashError',
94+
this.get('flashMessages').queue(
9395
`Version '${params.version_num}' of crate '${crate.get('name')}' does not exist`);
9496
}
9597

0 commit comments

Comments
 (0)