jagi / meteor-astronomy

Model layer for Meteor
https://atmospherejs.com/jagi/astronomy
MIT License
604 stars 67 forks source link

doc.isModified() does not work when a nested key is added to a document and the first level key itself does not exist #689

Open s7dhansh opened 6 years ago

s7dhansh commented 6 years ago

doc.isModified('status.active') works when status exists as an object, and fails when status is undefined. Is it intended? Seems to be a bug to me.

lukejagodzinski commented 6 years ago

@s7dhansh probably it's a bug, I will check it

lukejagodzinski commented 6 years ago

Fixed in jagi:astronomy@2.6.1

s7dhansh commented 6 years ago

@lukejagodzinski thanks. but now it has caused another bug. doc.isModified('apps.3.new') also turns out to be true now when modifying doc.apps.1. It's causing a lot of trouble. Reverted fro now.

lukejagodzinski commented 6 years ago

Hehe ehh that's what happens when you release bugfix quickly without testing it :P. Right, sorry I will fix that

lukejagodzinski commented 6 years ago

@s7dhansh sorry but in this case changing this behavior would require me to rewrite a lot of diffing algorithm and it can't be easily implemented. I will revert to the previous version.

s7dhansh commented 6 years ago

I checked your last couple of commits. Won't this work?

Updates code (modeled after something that I am using now).

const modified = _includes(modified, pattern);
if (!pattern.includes('.') || modified) return modified;
const segments = pattern.split('.');
let parent = '';
return segments.some((segment) => {
    parent += (parent ? '.' : '') + segment;
    return (doc.getModifier().$set || {})[parent] || (doc.getModifier().$unset || {})[parent];
});

I understand that there will not be just $set, but only $set and $unset should suffice as anything else would mean that the current field is definitely getting modified directly. I am not sure about the performance though, depends on how expensive is getModifier. But assuming you would be already calling it at least once for modification, you can cache the result and re-use it here.

lukejagodzinski commented 5 years ago

@s7dhansh from what I remember using getModifier should work but it's kinda ugly hack. The best way to solve this problem would be to modify diffing algorithm to include nested fields even when parent was unset or set. I will test it and let you know.