1.2.7 getModified regression

[marbemac]

We rely on getModified to show a save/undo button if there are changes to the model.

With 1.2.7, getModified doesn't work correctly for nested objects. Consider:

var model = {
    request: {
        method: 'get'
    }
}

// User changes request method on the model
model.set('request.method', 'put');

// Correctly says request has changed
console.log(model.getModified()); // Object {request: Class}

// User changes request method back
model.set('request.method', 'get');

// incorrectly says request is still modified, even though it is no longer different
console.log(model.getModified()); // Object {request: Class}

[marbemac]

In fact, model "dirty tracking" in general seems to have issues in 1.2.7.

[lukejagodzinski]

Can you create a reproduction?

When introducing 1.2.7 version I was expecting to get several issues about the getModified method and the _original property. It makes Astronomy much faster but changes a few things internally. I've tried to make as much backward compatible as possible. If I won't be able to fix it I would suggest forcing 1.2.6 version in your project. But let's just create reproduction and maybe I will be able to help.

[lukejagodzinski]

I've improved backward compatibility in Astronomy 1.2.8. You shouldn't have problem right now.

[marbemac]

I've updated my astronomy perf repo to demonstrate the bug:

https://github.com/marbemac/astronomy-perf

Load that meteor app up, then change the url in the input field. You'll see getModified() returns the entire request object, which is the first kind of weird thing.

Then, when you clear the input, isModified() correctly returns false, but getModified() still returns the request object, even though nothing in there is actually modified.

[lukejagodzinski]

Ok I know what's the problem. The reason that it still sees modifications is because document contains _modifiers even if the field's value was set to the original value. I will address that problem tomorrow and publish update. Sorry for breaking backward compatibility but I thought that making Astronomy faster is very important. I think that after that update everything should work as previously and in the same time much faster :)

[lukejagodzinski]

Fixed in 1.2.9

[marbemac]

👍 all good, thanks!

We continue to layer in hacks to work around #156. Any sort of progress update on 2.0? Not a huge rush, just curious.

Would a 2.0 umbrella issue make sense? Kind of like this:
facebook/react#3220

[lukejagodzinski]

I have a little delay with Astronomy 2.0 because of private stuff. Moreover there is Christmas now. So it will be at the beginning of 2016.

What is it about umbrella issue? I don't have time to read entire discussion.

[marbemac]

No problem on 2016.

I was just suggesting that it might make sense to create an umbrella issue for astronomy 2.0, similar to that linked issue. It's not strictly necessary - just helps people that are interested, follow the 2.0 progress without having to ask or bug you :).

[lukejagodzinski]

Oh I see. I didn't know that we can use the "umbrella" word in such situation :). Is there some reason that they use the "umbrella" word instead of something else? :)

Yes I think it's good idea to create such issue. I will do it.

[marbemac]

I hate to resurrect this again, but see the attached screenshot. I'm not sure exactly what is causing this to happen, but I'm seeing cases where getModified returns an empty object, but isModified() returns true. In the specific case I'm running into, there should be no modifications to the model (so getModified is working correctly, but isModified seems to have an issue).

I'll try and figure out how to reproduce this, but in general, should isModified ever return true if getModified returns an empty object?

[marbemac]

(this does not occur in 1.2.6)

[lukejagodzinski]

Ok I will take a look at it

[talha-asad]

@jagi They use the "umbrella" word because it kind of covers the entire thing. So like how an umbrella covers a person from rain in entirety.