-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Don't pass specific options to the parent constructor #3106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
||
this.setPreload(goog.isDef(options.preload) ? options.preload : 0); | ||
this.setUseInterimTilesOnError(goog.isDef(options.useInterimTilesOnError) ? | ||
options.useInterimTilesOnError : true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could probably do it w/o cloning the options object:
ol.layer.Tile = function(opt_options) {
var options = goog.isDef(opt_options) ? opt_options : {};
var preload = goog.isDef(options.preload) ? options.preload : 0;
var useInterimTilesOnError = goog.isDef(options.useInterimTilesOnError) ?
options.useInterimTilesOnError : true;
delete options.preload;
delete options.useInterimTilesOnError;
goog.base(this, /** @type {olx.layer.LayerOptions} */ (options));
this.setPreload(preload);
this.setUseInterimTilesOnError(useInterimTilesOnError);
};
(untested)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You code works but I suggest to keep the goog.object.clone
: it is how it's done in the other classes (ol.layer.Group
, ol.layer.Vector
, ...) and we will had to change if if/when we have a new non primitive type to handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You code works but I suggest to keep the goog.object.clone: it is how it's done in the other classes (ol.layer.Group, ol.layer.Vector, ...)
Ok.
and we will had to change if if/when we have a new non primitive type to handle.
We'd need to add delete
statements anyway, even if clone
is used. No?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd need to add delete statements anyway, even if clone is used. No?
Correct.
I thought that deleting a non primitive type from an object will delete the copy too:
var obj = {
arr: [1,2,3]
};
var c = obj.arr;
delete obj.arr;
Unlike what I was thinking: c == [1,2,3]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete
just removes the property from an object. It does nothing to the object that property refers to.
@elemoine do you agree to merge this PR as it? |
Yep. Please merge. |
Don't pass specific options to the parent constructor
Same as #3104