fix(core): stop mutating Context's input#3076
Conversation
lib/core/base/context.js
Outdated
| ? Array.from(context.include) | ||
| : [document], | ||
| exclude: context.exclude || [] | ||
| exclude: context.exclude ? clone(context.exclude) : [] |
There was a problem hiding this comment.
This isn't going to work. Exclude can contain DOM nodes. We can't clone DOM elements.
There was a problem hiding this comment.
Looks like DOM nodes aren't being cloned.
Co-authored-by: Stephen Mathieson <me@stephenmathieson.com>
There was a problem hiding this comment.
I think we should change the approach. Instead of trying to remember that we should clone every property of the context that may be an object (one missed here), we should just clone the context object upfront as the first thing of Context function.
export function Context(spec, flatTree) {
spec = clone(spec);
}Then we should also test is that the context returned from new Context doesn't strict equal any parts of the old context. Even if the context function itself doesn't mutate the object, that context is used in a ton of places so we don't want to share any memory with the original spec.
// test/context.js
it('should not share memory with complex object', function() {
var spec = {
include: [['#header'], ['a'], ['#frame1', '#frame2', '#fix']],
exclude: [['iframe', '#foo']] ,
size: { width: 100, height: 100 }
};
var context = new Context(spec);
assert.notStrictEqual(spec.include, context.include);
spec.include.forEach(function(_, index) {
assert.notStrictEqual(spec.include[index], context.include[index]);
});
assert.notStrictEqual(spec.exclude, context.exclude);
spec.exclude.forEach(function(_, index) {
assert.notStrictEqual(spec.exclude[index], context.exclude[index]);
});
assert.notStrictEqual(spec.size, context.size);
});
it('should not share memory with simple array', function() {
var spec = ['#f1']
var context = new Context(spec);
assert.notStrictEqual(spec.include, context.include);
});Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
|
@straker because of the dom node cloning issue we won't be able to ensure that the node passed in to context (if one was) is not the same node as Context is using. In fact it always will be the same node. |
|
Correct. For the array include/exclude we should ensure no memory sharing but that can't be helped for a single DOM node. |
Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
getFrameContexts was mutating its input because Context mutates its input. This change makes it so Context no longer mutates its input.
Closes issue: #3045