Skip to content

Conversation

@navelpluisje
Copy link

Resolves #10
Added changed, removed and added events back to the observer.
Added the fields which have been added to the response of the observer.

Added the fields which have been added to the response of the observer.
index.js Outdated
Copy link

Choose a reason for hiding this comment

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

@navelpluisje pardon me if I'm mistaken, but by this point in the code, newFields is still an empty object right? If you look here in the master branch, we add every key-value pair to newFields and then pass it into observer.changed.

In this case however, I believe just passing in item in place of newFields would suffice as it is on line 191 here in your PR.

Copy link

Choose a reason for hiding this comment

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

Also, I can't leave a line note on line 466, but the changed DDP message's observer function should be renamed back to changed from updated to be consistent with the DDP message type and the code you've written here.

Copy link
Author

Choose a reason for hiding this comment

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

@sunny-g Thanx for mentioning about the newFields. I will submit a change in a minute. Like you mentioned it is possible to set item as the parameter, but if the submit to the cache fails this is not the right value. I'm setting the value of newFields with the result of the upsert. Just forgot to copy this part;-)

@sunny-g
Copy link

sunny-g commented Oct 17, 2015

Sorry if I'm coming off as a pedant (I was working on the same fixes but hadn't gotten around to making the PR), the last thing I'dd add to this PR is bumping the minor version of the lib to 0.1.3 in the package.json so that it can be republished by @hharnisc as soon as its merged.

@navelpluisje
Copy link
Author

I think the versioning has to be done by @hharnisc, this because he's the maintainer.
About the code @ line 466 it is just as it is in the master branche. For that reason I left t as it is.

@sunny-g
Copy link

sunny-g commented Oct 17, 2015

@navelpluisje I guess you're right about the versioning, though you can't republish the same version to NPM, you'd have to bump at least the minor version.

The change I'm referencing on line 466 is changing the passed in parameter to changed and the observe function's name to changed from updated since changed is what you called in your PR and that's what the DDP message type is.

@navelpluisje
Copy link
Author

@sunny-g In DDP there's a difference between updated and changed. You can read more about it here https://github.com/meteor/.

@sunny-g
Copy link

sunny-g commented Oct 17, 2015

@navelpluisje I know, changed refers to changes in the subscription whereas updated refers to Meteor's call and RPC functionality: DDP.md. You called observer.changed in your PR, but the observer still has an updated method attached to it by default within the observe function.

Compare: https://github.com/hharnisc/node-ddp-client/blob/master/index.js#L470 to https://github.com/hharnisc/node-ddp-client/pull/11/files#diff-168726dbe96b3ce427e7fedce31bb0bcR236

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.

2 participants