{"action":"created","issue":{"url":"https://api.github.com/repos/BranchMetrics/Web-SDK/issues/8","labels_url":"https://api.github.com/repos/BranchMetrics/Web-SDK/issues/8/labels{/name}","comments_url":"https://api.github.com/repos/BranchMetrics/Web-SDK/issues/8/comments","events_url":"https://api.github.com/repos/BranchMetrics/Web-SDK/issues/8/events","html_url":"https://github.com/BranchMetrics/Web-SDK/pull/8","id":56875082,"number":8,"title":"Version 2","user":{"login":"scotthasbrouck","id":4266945,"avatar_url":"https://avatars.githubusercontent.com/u/4266945?v=3","gravatar_id":"","url":"https://api.github.com/users/scotthasbrouck","html_url":"https://github.com/scotthasbrouck","followers_url":"https://api.github.com/users/scotthasbrouck/followers","following_url":"https://api.github.com/users/scotthasbrouck/following{/other_user}","gists_url":"https://api.github.com/users/scotthasbrouck/gists{/gist_id}","starred_url":"https://api.github.com/users/scotthasbrouck/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/scotthasbrouck/subscriptions","organizations_url":"https://api.github.com/users/scotthasbrouck/orgs","repos_url":"https://api.github.com/users/scotthasbrouck/repos","events_url":"https://api.github.com/users/scotthasbrouck/events{/privacy}","received_events_url":"https://api.github.com/users/scotthasbrouck/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":5,"created_at":"2015-02-06T22:09:15Z","updated_at":"2015-02-17T20:18:34Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/BranchMetrics/Web-SDK/pulls/8","html_url":"https://github.com/BranchMetrics/Web-SDK/pull/8","diff_url":"https://github.com/BranchMetrics/Web-SDK/pull/8.diff","patch_url":"https://github.com/BranchMetrics/Web-SDK/pull/8.patch"},"body":""},"comment":{"url":"https://api.github.com/repos/BranchMetrics/Web-SDK/issues/comments/74744216","html_url":"https://github.com/BranchMetrics/Web-SDK/pull/8#issuecomment-74744216","issue_url":"https://api.github.com/repos/BranchMetrics/Web-SDK/issues/8","id":74744216,"user":{"login":"dmitrig01","id":10422,"avatar_url":"https://avatars.githubusercontent.com/u/10422?v=3","gravatar_id":"","url":"https://api.github.com/users/dmitrig01","html_url":"https://github.com/dmitrig01","followers_url":"https://api.github.com/users/dmitrig01/followers","following_url":"https://api.github.com/users/dmitrig01/following{/other_user}","gists_url":"https://api.github.com/users/dmitrig01/gists{/gist_id}","starred_url":"https://api.github.com/users/dmitrig01/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dmitrig01/subscriptions","organizations_url":"https://api.github.com/users/dmitrig01/orgs","repos_url":"https://api.github.com/users/dmitrig01/repos","events_url":"https://api.github.com/users/dmitrig01/events{/privacy}","received_events_url":"https://api.github.com/users/dmitrig01/received_events","type":"User","site_admin":false},"created_at":"2015-02-17T20:18:34Z","updated_at":"2015-02-17T20:18:34Z","body":"doing some review\r\n\r\n### readme (and API I guess):\r\n\r\n- [ ] several typos: \"Requriements\", \"app_di\", \"queed\", \"accountrmation\", \"imrpovements\", \"wether\" (this was a more goaty product, but alas...) etc. let's get someone to read over this and spell/grammar/make-sense check when it's done - maybe @derrickstaten?\r\n- [ ] very minor, but the Xes look ilke we don't support it; perhaps a check mark? :-)\r\n- [ ] some extreme/funky indentation, no?\r\n- [ ] most of these functions return nothing - what you classify as \"returns\" is actually the data passed to the callback, right?\r\n- [ ] I know the server-side call is named `profile` but I think it would make more sense as `identify` (like Mixpanel) or `setIdentity` (like our SDKs)\r\n- [ ] not sure how much control we have over jsdoc formatting, but \"'init'(app_id, callback)\" looks weird. There are a bunch of ways of denoting this; the haskell/loosely-category-theoretical way would be something like `init :: app_id, ((err, data) -> null) -> null` but not sure if this is the most intuitive way for JS developers. Let's find some good reference docs out there and copy them\r\n- [ ] `Note: Branch.init is called every time the constructor is loaded.` this doesn't make sense - what do you mean by \"constructor is loaded\"? Are you trying to say `Note: you must call Branch.init before using any other Branch SDK method`?\r\n- [ ] for `init` let's be more clear that \"this is where you can read the data about the link the user used to get to this page\". i'm not sure we need to return all that data to the user - let's only return them stuff they need: data and referring_identity\r\n- [ ] on the profile call - let's include a little more info on what exactly the identity string is for\r\n- [ ] maybe we should have some kind of more overviewey section that says something along the lines of \"to properly work with user tracking, you will have to call our SDK on login and logout events; do that with `identify` and `logout`\"\r\n- [ ] still not sure of a use-case for `close` - i'd say let's remove it (or leave it in but commented out)\r\n- [ ] for `link` - let's \"return\" just the link, not that object. call the parameter `data` not `metadata`. let's include some docs on what that parameter is - we can copy from the iOS or Android guides, it's pretty well documented\r\n- [ ] i might keep `event` as `track`, as it's in line with what mixpanel does - what do you think?\r\n- [ ] `SMSLink` - still not sure on the name - how about `sendSMS`? And let's be super clear in the description: *if the user got to this page from a branch link, send that same link; otherwise, create a new link with the data provided here*. would be great to make the link parameter optional too, and if you omit it, it just sends a link with everything default and channel SMS...\r\n- [ ] would be great to be able to pass `SMSLink` a string (of the form `bnc.lt/l/XYZ` or some kind of white-labelled link [for now, we can just assume it's a branch link? can also validate it's the form `http(s?)://(domain name)/l/[A-Za-z0-9_\\-]+`) so it doesn't create a new link every time. maybe this is scope creep for now?\r\n- [ ] `SMSLinkNew` - also unsure about the name. now that I'm seeing it laid out here, I think it makes more sense as an optional boolean flag to sendSMS: `use_referring_link_if_exists` or something ugly like that, defaulting to true? (but optional) (same unease for `SMSLinkExisting`; not sure if this function makes sense as a standalone)\r\n- [ ] `credits` and `referrals` - i know this is a bit redundant, but can we copy some text on what these actually show from the iOS or Android docs?\r\n- [ ] `redeem` - let's pass it two params, `bucket` and `amount`. looks like it's not documented as getting (err, data)?\r\n- [ ] `banner` - love it. would love to be more thorough on docs here:\r\n  - [ ] screenshot!\r\n  - [ ] let's pull this data from the branch dashboard if you've set it - i'll work on the server-side piece to do that\r\n  - [ ] let's document better what these parameters are. it looks like the data sub-object _within_ this object is the link parmaeters (and that itself can have a data object)? what about splitting out this into a second parameter, with a similar format to SMS - you can pass a link data, link string, or nothing? and it can make the channel 'app banner' by default...\r\n  - [ ] does the callback get `data` or `err, data`? is the callback useful to the developer?\r\n  - [ ] are we going to have options for mobile-only, mobile-and-desktop, desktop-only?\r\n\r\n### Overall:\r\n\r\n- [ ] we used to have branch.utils.readSession() which people would use to get the referring data. we sholud probably provide some method to replace that. it would be awesome to make it an async method, so the function would be like: load data if it's not there (i.e. waiting for an init call - really, the same as an init call, but without the app id, because init has to already have been called) or return it if it's there\r\n- [ ] we need a request queueing system - i can fill in more details on this - but the gist is: certain calls (init, identify, logout) can change the identity_id and/or session id; if you do something like `branch.track('a'); branch.logout(); branch.track('b');`, we want the second event to get tracked with the post-logout session id. does that make sense? there are a couple of ways of doing this. the easiest way is probably just having a queue of requests so when you call `this.api` in the branch object, instead of executing, it adds to the queue (or executes immediately if the queue is empty); when execution finishes, it calls the next item on the queue. that's why we add in identity_id, session_id, and app_id _separately_ - because it lets us do this very easily\r\n- [ ] some code formatting, styling, indentation, etc. inconsistencies. Looks like I have jshint in gulp, we should make sure to run that. I'll add JSCS as well.\r\n\r\n### Code specifics\r\n\r\n- [ ] `1_elements.js` maybe better named `1_banner`?\r\n- [ ] let's take `Branch.prototype.api` off the branch prototype since it's an internal function\r\n- [ ] let's make sure to add back IE9 and IE10 specific fixes\r\n- [ ] doesn't your `bannerShown` actually mean `bannerHidden`?\r\n- [ ] can you clarify your comment `message; //Does not work with tests`? let's work this out in chat\r\n- [ ] `referrals` and `credits` - would be great to cache these client-side (we can do it per-page load right now, not in the session cache)\r\n- [ ] i saw you updated the on-page script; let's update the source of the script - `src/onpage.js` - and have a build step which minifies it puts this in the docs\r\n\r\n### Tests:\r\n\r\n- [ ] for testing - the tests are good as-is, but I would like to see another type of test as well: it would be great to stub out server calls so we can: (a) make sure requests are sent to the server in the proper format, and (b) simulate different responses. Specifically, to do this, we would need:\r\n  - [ ] In `1_api`, separate the request-building part (https://github.com/BranchMetrics/Web-SDK/blob/implements_sms/src/1_api.js#L85-L99) from the XHR part (rest of that function).\r\n  - [ ] Add a mocked/stubbed XHR-runner function\r\n  - [ ] start to write some basic tests that the SDK is actually sending requests in the right format; simulate some errors as well (invalid app ID, for example) and see how they are passed back\r\n  - [ ] more testing along that line: would be great to use this to test the request queueing type thing I mentioned above, as we can do exactly the calls i had above, and ensure the second event is tracked with the second session id (fake-returned by our mocked logout api call).\r\n- [ ] let's add some docs or automated tooling around running tests. those relative paths in tests/branch_tests.js look janky :-)\r\n\r\n### Release:\r\n\r\n- [ ] let's write up a release process, (and even think about turning into a script?!)\r\n- [ ] let's release it as 1.0.0!"}}