Skip to content
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

fix(core): stop mutating Context's input #3076

Merged
merged 12 commits into from
Jul 15, 2021

Conversation

clottman
Copy link
Contributor

getFrameContexts was mutating its input because Context mutates its input. This change makes it so Context no longer mutates its input.

Closes issue: #3045

@clottman clottman requested a review from a team as a code owner July 12, 2021 22:47
: [document],
exclude: context.exclude || []
exclude: context.exclude ? clone(context.exclude) : []
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't going to work. Exclude can contain DOM nodes. We can't clone DOM elements.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like DOM nodes aren't being cloned.

Co-authored-by: Stephen Mathieson <[email protected]>
@clottman clottman dismissed WilcoFiers’s stale review July 13, 2021 14:16

addressed in comments

@clottman clottman requested a review from WilcoFiers July 13, 2021 14:16
Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

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 <[email protected]>
@clottman
Copy link
Contributor Author

@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.

@straker
Copy link
Contributor

straker commented Jul 13, 2021

Correct. For the array include/exclude we should ensure no memory sharing but that can't be helped for a single DOM node.

lib/core/utils/clone.js Outdated Show resolved Hide resolved
@straker straker merged commit 5dc34ee into develop Jul 15, 2021
@straker straker deleted the 3045-stop-mutating-context-input branch July 15, 2021 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants