{"action":"created","issue":{"url":"https://api.github.com/repos/facebook/AsyncDisplayKit/issues/2782","repository_url":"https://api.github.com/repos/facebook/AsyncDisplayKit","labels_url":"https://api.github.com/repos/facebook/AsyncDisplayKit/issues/2782/labels{/name}","comments_url":"https://api.github.com/repos/facebook/AsyncDisplayKit/issues/2782/comments","events_url":"https://api.github.com/repos/facebook/AsyncDisplayKit/issues/2782/events","html_url":"https://github.com/facebook/AsyncDisplayKit/pull/2782","id":195919028,"number":2782,"title":"[ASNetworkImageNode] Assert if both URL and image are set","user":{"login":"garrettmoon","id":90045,"avatar_url":"https://avatars.githubusercontent.com/u/90045?v=3","gravatar_id":"","url":"https://api.github.com/users/garrettmoon","html_url":"https://github.com/garrettmoon","followers_url":"https://api.github.com/users/garrettmoon/followers","following_url":"https://api.github.com/users/garrettmoon/following{/other_user}","gists_url":"https://api.github.com/users/garrettmoon/gists{/gist_id}","starred_url":"https://api.github.com/users/garrettmoon/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/garrettmoon/subscriptions","organizations_url":"https://api.github.com/users/garrettmoon/orgs","repos_url":"https://api.github.com/users/garrettmoon/repos","events_url":"https://api.github.com/users/garrettmoon/events{/privacy}","received_events_url":"https://api.github.com/users/garrettmoon/received_events","type":"User","site_admin":false},"labels":[{"id":196858465,"url":"https://api.github.com/repos/facebook/AsyncDisplayKit/labels/CLA%20Signed","name":"CLA Signed","color":"bfe5bf","default":false},{"id":284919650,"url":"https://api.github.com/repos/facebook/AsyncDisplayKit/labels/Development%20in%20Progress","name":"Development in Progress","color":"eb6420","default":false},{"id":479382409,"url":"https://api.github.com/repos/facebook/AsyncDisplayKit/labels/Image%20&%20Video","name":"Image & Video","color":"d4c5f9","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2016-12-15T21:21:36Z","updated_at":"2016-12-21T10:48:38Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/facebook/AsyncDisplayKit/pulls/2782","html_url":"https://github.com/facebook/AsyncDisplayKit/pull/2782","diff_url":"https://github.com/facebook/AsyncDisplayKit/pull/2782.diff","patch_url":"https://github.com/facebook/AsyncDisplayKit/pull/2782.patch"},"body":"Here's one other idea for addressing this problem. Essentially,\r\nwe assert if you've set both the URL and the image. I've played around\r\nwith Pinterest and there's only one case where we hit this (the transition).\r\n\r\nSo I've also added another method (which is a bummer, it's weird I know)\r\nbut there's one good reason to add this method, ephemeralImage, which is\r\nthe user doesn't have to manually clear it out like they would if they\r\nused defaultImage to save memory.\r\n\r\nPutting this up for discussion."},"comment":{"url":"https://api.github.com/repos/facebook/AsyncDisplayKit/issues/comments/268493020","html_url":"https://github.com/facebook/AsyncDisplayKit/pull/2782#issuecomment-268493020","issue_url":"https://api.github.com/repos/facebook/AsyncDisplayKit/issues/2782","id":268493020,"user":{"login":"maicki","id":25288,"avatar_url":"https://avatars.githubusercontent.com/u/25288?v=3","gravatar_id":"","url":"https://api.github.com/users/maicki","html_url":"https://github.com/maicki","followers_url":"https://api.github.com/users/maicki/followers","following_url":"https://api.github.com/users/maicki/following{/other_user}","gists_url":"https://api.github.com/users/maicki/gists{/gist_id}","starred_url":"https://api.github.com/users/maicki/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/maicki/subscriptions","organizations_url":"https://api.github.com/users/maicki/orgs","repos_url":"https://api.github.com/users/maicki/repos","events_url":"https://api.github.com/users/maicki/events{/privacy}","received_events_url":"https://api.github.com/users/maicki/received_events","type":"User","site_admin":false},"created_at":"2016-12-21T10:48:38Z","updated_at":"2016-12-21T10:48:38Z","body":"As already discussed extensively with @garrettmoon my thoughts around the behavior of each of the cases for an `ASNetworkImageNode` are:\r\n\r\n### a. Just .URL\r\nAgree with @appleguy ... no changes: This behavior is simple. It should not change from the prior implementation. If externally read, the .image property will become set after the URL has loaded. Likewise, the rendered version will be cleared when exiting the Display range, and the .image property itself will be cleared existing the Preload range.\r\n\r\n### b. Just .image\r\nAgree with @appleguy: This behavior is also simple, but doesn't work correctly in 2.0. When just the .image is set and .URL is never set, the class should behave like ASImageNode.\r\n\r\n### c. Both .URL and .image will be set\r\nIn the case that a client set's an URL and an image it's less clear, but the thinking is to reduce the state and the side effects it can have it should actually work this:\r\n1. If a URL is set it behaves like an `ASNetworkImageNode`. If an image should show before the image did load from the network it should be assigned to the `defaultImage` property and *not* the image property. The same way as the `ASNetworkImageNode` works as of today. I don't see any reason why we should support setting an `.image` as temporary image if there is the `defaultImage` too ... that said `defaultImage` is a really poor name for what it actually is: a placeholder image before the image did load from the network\r\n2. As soon an image is assigned (and a URL was set before) the `ASNetworkImageNode` will be transformed to the an `ASImageNode` and the `URL` will be discarded.\r\n3. If an `.image` is set and a `URL` will be set  the node will be transformed from an `ASImageNode` to an `ASNetworkImageNode`. The `.image` will be removed replaced with the `defaultImage` until the image did load from the network and replaces the `defaultImage` as `image`.\r\n\r\n### Why this behavior?\r\nImho, the behavior above is clear to describe as well hopefully reduces side effects for all kinds of quirks that can happen and added complexity we have to implement in case we want to support setting and `.image` as well as an `URL` and if someone is crazy also setting a `defaultImage` at the same time.\r\n\r\n### Summary\r\nTo summarize the states:\r\n1. Setting a `URL` the node will become an `ASNetworkImageNode` the same way as it works currently. If the developer would like to display an image before the final image did load from the network, it should use the `defaultImage` as it's currently the case too. All previous set image will be discarded.\r\n2. Setting an image directly to the `image` property, the node will become an `ASImageNode` with resetting all previous state like the `defaultImage` or the `URL` that was maybe set previously."}}
{"actor":{"display_login":"maicki"}}