Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

Commit

Permalink
fix(compiler): prevent resolve and locals overwrite
Browse files Browse the repository at this point in the history


Objects passed to a function should be considered immutable

 to avoid unexpected side-effects

Closes #2676. Fixes #2614.
  • Loading branch information
rochdev authored and ThomasBurleson committed Jul 5, 2015
1 parent 9434120 commit bee0523
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 20 deletions.
4 changes: 2 additions & 2 deletions src/core/services/compiler/compiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ function mdCompilerService($q, $http, $injector, $compile, $controller, $templat
var template = options.template || '';
var controller = options.controller;
var controllerAs = options.controllerAs;
var resolve = options.resolve || {};
var locals = options.locals || {};
var resolve = angular.copy(options.resolve || {});
var locals = angular.copy(options.locals || {});

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jul 9, 2015

Member

Maybe using angular.extend({}, options.resolve/locals || {}) would be the best of both worlds (allowing passing objects by reference and not modifying the original resolve/locals object.

var transformTemplate = options.transformTemplate || angular.identity;
var bindToController = options.bindToController;

Expand Down
50 changes: 32 additions & 18 deletions src/core/services/compiler/compiler.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,27 +54,41 @@ describe('$mdCompiler service', function() {
expect(data.element.html()).toBe('hello world');
});

it('resolve and locals should work', function() {
module(function($provide) {
$provide.constant('StrawberryColor', 'red');
});
describe('with resolve and locals options', function() {
var options;

var data = compile({
resolve: {
//Resolve a factory inline
fruit: function($q) {
return $q.when('apple');
beforeEach(function() {
module(function($provide) {
$provide.constant('StrawberryColor', 'red');
});

options = {
resolve: {
//Resolve a factory inline
fruit: function($q) {
return $q.when('apple');
},
//Resolve a DI token's value
color: 'StrawberryColor'
},
//Resolve a DI token's value
color: 'StrawberryColor'
},
locals: {
vegetable: 'carrot'
}
locals: {
vegetable: 'carrot'
}
};
});

it('should work', function() {
var data = compile(options);
expect(data.locals.fruit).toBe('apple');
expect(data.locals.vegetable).toBe('carrot');
expect(data.locals.color).toBe('red');
});

it('should not overwrite the original values', function() {
var clone = angular.copy(options);
compile(options);
expect(options).toEqual(clone);
});
expect(data.locals.fruit).toBe('apple');
expect(data.locals.vegetable).toBe('carrot');
expect(data.locals.color).toBe('red');
});

describe('after link()', function() {
Expand Down

2 comments on commit bee0523

@hodeyp
Copy link

@hodeyp hodeyp commented on bee0523 Jul 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to me to be a good idea. It is now impossible to pass objects by reference to an md-dialog and have that object returned in the md-dialog promise. Have raised a new issue regarding this: #3651

@no-more
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

I'm not sure why this was changed. I can't pass promise any more to the dialog. But mainly I don't understand why in input should be immutable. For me it should behave as in a directive call. If you don't want to change the object you have to handle the copy yourself before passing to the call, or inside the controller. It's up to the developer. Now you can't do that anymore and you have to implement more complex "callback" function if you want to change the input parameter. I don't think this is the good way to do it.

Thanks.

Please sign in to comment.