8000 Switch to passing only the current-change in 'changes' emit by pik · Pull Request #387 · js-data/js-data · GitHub
[go: up one dir, main page]

Skip to content

Switch to passing only the current-change in 'changes' emit #387

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

Merged
merged 3 commits into from
Aug 11, 2016

Conversation

pik
Copy link
Contributor
@pik pik commented Aug 8, 2016

This changes the diff function to use the previous prop, value pair rather than the _get('previous') which always diffs the committed state. #372

One difference that accompanies this change is that while before a single changeHistory() entry contained all the changes to get to that history from previous now they would need to be merged. I'm not sure what the general practical applications for changeHistory are, so I don't know how having sparse vs. full-objects affects those.

@pik pik force-pushed the change-history branch 3 times, most recently from 47e5ed0 to d0935c6 Compare August 8, 2016 00:12
@pik
Copy link
Contributor Author
pik commented Aug 8, 2016

@jmdobry Added some comments in the code for points of interest (will extract / force-push them out later).

@pik
Copy link
Contributor Author
pik commented Aug 8, 2016

Also added noChangeHistoryPath, maybe also worth enabling this on mapper / mapperDefaults ?

@pik pik force-pushed the change-history branch from 302031a to 7fe8ff9 Compare August 8, 2016 00:45
const changeHistory = _get(changeHistoryPath) || []
_set(changeHistoryPath, changeHistory)
changeHistory.push(changeRecord)
if (!this[noChangeHistoryPath]) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be

if (!_get(noChangeHistoryPath)) {

@jmdobry
Copy link
Member
jmdobry commented Aug 8, 2016

Can add a keepChangeHistory option to the Mapper class, which would default to true. A record can check that property of its associated mapper.

assert.equal(Object.keys(changes.removed).length, 0)
assert.equal(changes.changed.name, 'updated foo', "Only the property changed was emitted in the changeSet")
done()
}, 5)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw the other tests using timeout - but something like an eventloop.after(function() {}) would be a preferable approach for testing, not sure how difficult that is to do ?

Copy link
Member

Choose a reason for hiding this comment

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

In this case, something like this might work:

setImmediate(function () {...});

however, the tests need to work in a bunch of different browsers, and I'm not sure how cross-browser setImmediate is, so setTimeout might be just be good enough. See https://github.com/js-data/js-data/blob/master/karma.conf.js#L3

@pik
Copy link
Contributor Author
pik commented Aug 8, 2016

re:

Can add a keepChangeHistory option to the Mapper class, which would default to true. A record can check that property of its associated mapper.

Should there be some standard interface for passing mapper options down to the record? e.g. recordDefaults? atm for example mapper has noValidate as an option but there is no permanent way to pass that down either.

#378 (comment)

@pik pik force-pushed the change-history branch 3 times, most recently from 4b60d8a to 1981677 Compare August 8, 2016 04:58
@jmdobry
Copy link
Member
jmdobry commented Aug 8, 2016

That noValidate option is used by the Mapper's lifecycle hooks, and isn't used by the Record class.

If a record wants to know what the keepChangeHistory settings is for its associated mapper, it would get the value of the setting like this: this._mapper().keepChangeHistory.

@pik
Copy link
Contributor Author
pik commented Aug 8, 2016

@jmdobry I see, so you'd prefer that changing keepChangeHistory on the mapper dynamically changes record behavior (and should this be lower priority than setting it in the constructor or not) ?

@jmdobry
Copy link
Member
jmdobry commented Aug 8, 2016

I was thinking that the record would look at the mapper's setting just once, when the record is constructed. After the record is constructed, you'd control the setting on the record itself. Does that sound okay?

@pik
Copy link
Contributor Author
pik commented Aug 10, 2016

@pik pik force-pushed the change-history branch 2 times, most recently from d4ba485 to 9439fc8 Compare August 10, 2016 02:53
@jmdobry
Copy link
Member
jmdobry commented Aug 11, 2016

LGTM, will merge tomorrow.

 * check that mapper is defined before checking keepChangeHistory
}),

/* The previous test has a property set and changed back within a single event loop
* So no listener is ever called.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added a code comment to clarify the reason for adding the second test.

@jmdobry jmdobry merged commit 489620c into js-data:master Aug 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0