Skip to content
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

[BUGFIX Beta] unwatch should not create a new meta #12492

Closed
wants to merge 1 commit into from

Conversation

stefanpenner
Copy link
Member

  • m.writeWatching(keyName, count - 1); does require meta, ensure by then we have meta + add covering test...

@@ -102,13 +104,19 @@ export function unwatchKey(obj, keyName, meta) {
enumerable: true,
value: val
});
m.deleteFromValues(keyName);

var m = peekMeta(obj);
Copy link
Member

Choose a reason for hiding this comment

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

This definitely seems correct, but shadowing the outer 'm' feels odd to me. Can we use either the variable declared above (just inside the main unwatchKey function) or pick a different variable name than 'm'?

Copy link
Member Author

Choose a reason for hiding this comment

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

meant to write let, 1 sec

@stefanpenner stefanpenner changed the title [BUGFIX Release] unwatch should not create a new meta [BUGFIX Beta] unwatch should not create a new meta Oct 16, 2015
@krisselden
Copy link
Contributor

unwatch is not supposed to be ever called on something that wasn't watched, can you elaborate on the root cause?

@stefanpenner
Copy link
Member Author

can you elaborate on the root cause?

nope

unwatch is not supposed to be ever called on something that wasn't watched

I'll change it to an assert, and write a test on the inner get always getting its meta on-demand.

@stefanpenner
Copy link
Member Author

superseded by #12491

@stefanpenner stefanpenner deleted the unwatch-no-meta branch October 18, 2015 15:12
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.

3 participants