Skip to content

Create previousAttributes using a deep clone of attributes#1876

Merged
ricardograca merged 1 commit into
bookshelf:masterfrom
okuyiga:feature/fix-nested-json-bug
Nov 4, 2018
Merged

Create previousAttributes using a deep clone of attributes#1876
ricardograca merged 1 commit into
bookshelf:masterfrom
okuyiga:feature/fix-nested-json-bug

Conversation

@okuyiga

@okuyiga okuyiga commented Jul 8, 2018

Copy link
Copy Markdown

Introduction

Altered Model._reset to use a deep clone so that changes to Model.attributes don't affect Model._previousAttributes.

Motivation

Problem is detailed in #1875.

Proposed solution

Use _.cloneDeep. Fixes #1875.

Current PR Issues

None

Alternatives considered

None

@ricardograca ricardograca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Everything looks good except for the test description which seems a bit misleading and could lead to confusion further down the road.

Comment thread test/integration/json.js
});
});

it('can update attributes without affecting _previousAttributes', function() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure this description fits with what is actually being tested here. It should be the opposite since previousAttributes will now detect changes to nested objects.

@ricardograca

Copy link
Copy Markdown
Member

I'm going to merge this as is and then change the description of the test in another PR.

@ricardograca
ricardograca merged commit 32add1b into bookshelf:master Nov 4, 2018
@ricardograca

Copy link
Copy Markdown
Member

Renamed in fc66733.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants