{"action":"opened","issue":{"url":"https://api.github.com/repos/kmalakoff/knockback/issues/155","repository_url":"https://api.github.com/repos/kmalakoff/knockback","labels_url":"https://api.github.com/repos/kmalakoff/knockback/issues/155/labels{/name}","comments_url":"https://api.github.com/repos/kmalakoff/knockback/issues/155/comments","events_url":"https://api.github.com/repos/kmalakoff/knockback/issues/155/events","html_url":"https://github.com/kmalakoff/knockback/issues/155","id":159305480,"number":155,"title":"kb.Store::retainOrCreate() calls kb.Store::retain() twice when 'creating' and never when 'retaining'","user":{"login":"piusatst","id":2148699,"avatar_url":"https://avatars.githubusercontent.com/u/2148699?v=3","gravatar_id":"","url":"https://api.github.com/users/piusatst","html_url":"https://github.com/piusatst","followers_url":"https://api.github.com/users/piusatst/followers","following_url":"https://api.github.com/users/piusatst/following{/other_user}","gists_url":"https://api.github.com/users/piusatst/gists{/gist_id}","starred_url":"https://api.github.com/users/piusatst/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/piusatst/subscriptions","organizations_url":"https://api.github.com/users/piusatst/orgs","repos_url":"https://api.github.com/users/piusatst/repos","events_url":"https://api.github.com/users/piusatst/events{/privacy}","received_events_url":"https://api.github.com/users/piusatst/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":0,"created_at":"2016-06-09T00:24:44Z","updated_at":"2016-06-09T00:24:44Z","closed_at":null,"body":"Hi kmalalkoff, \r\n\r\nI'm a big fan of this library. There's just one thing which is breaking our very complex app.\r\n\r\nThe quick version of this issue, is that `retain()` is called twice when a new viewModel is registered with `retainOrCreate` (once at the end of this function, line 112; and once when the viewModel is created, via `useOptionsOrCreate()`, line 33). When a viewModel already exists, i expect retain to be called, but instead the function just returns the found observable, line 103.\r\n* The lines above refer to src/core/store.coffee.\r\n\r\nI'm not sure if this is how you intended to use the retain function, but let me tell why i expect this behaviour.\r\n\r\nMy complex App uses a **global kb.Store** to share all my viewModels.\r\nMany of my models have a  complex hierarchy of nested models (no circular structures) and sometimes I want to set a nested model, to another model. When I set a nested model to another one, the previous value gets released. Quite often that previous value is a shared viewModel which appears in another collectionObservable or nested in another model. Each time a shared ViewModel is released, the refCount is reduced until it is eventually destroyed, breaking the app. \r\n\r\nThink of this simple scenario. Job is a viewModel, Client is a viewModel, Job has a Client as a nested model. User changes JobA.client() from ClientA to ClientB. ClientA gets destroyed and any other Jobs that held that client now references a __kb_released ViewModel. (* More accurately, you need to change the client back and forth twice because the ref_count starts at 2.)\r\n\r\nTheres an easy way to fix this problem. 1) add a ref_count using retain whenever a viewModel is created or set on a parent viewModel (or simply call retain() when an observable is found in `retainOrCreate`). The double ref_count on creation is a less significant problem, but seems like a bug. \r\n\r\nI've been logging the ref_count changes in some of my tests and i expect the changes to look like 1, 2, 3, 4, 3, 2, 1, destroyed. But they actually look like 2, 2, 2, 2, 1, destroyed.\r\n\r\nNote that this only happens when I specify a custom global kb.Store on all my viewModels. If I don't do this and let the Framework create the stores for me, the viewModels don't get shared and way too many viewModels get created, freezing up the browser significantly.\r\n\r\nI know the change that needs to be made to retainOrCreate (call retain when existing observable found, but not otherwise). Let me know if you agree and would like me to fork and send a pull request.\r\n\r\nThis issue is a bit of a show stopper for us, but everything else about this framework is like poetry. Great work!"}}
{"actor":{"display_login":"piusatst"}}