{"action":"created","issue":{"url":"https://api.github.com/repos/mozilla/browser.html/issues/681","labels_url":"https://api.github.com/repos/mozilla/browser.html/issues/681/labels{/name}","comments_url":"https://api.github.com/repos/mozilla/browser.html/issues/681/comments","events_url":"https://api.github.com/repos/mozilla/browser.html/issues/681/events","html_url":"https://github.com/mozilla/browser.html/pull/681","id":112910582,"number":681,"title":"virtual-dom integration work","user":{"login":"Gozala","id":21236,"avatar_url":"https://avatars.githubusercontent.com/u/21236?v=3","gravatar_id":"","url":"https://api.github.com/users/Gozala","html_url":"https://github.com/Gozala","followers_url":"https://api.github.com/users/Gozala/followers","following_url":"https://api.github.com/users/Gozala/following{/other_user}","gists_url":"https://api.github.com/users/Gozala/gists{/gist_id}","starred_url":"https://api.github.com/users/Gozala/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Gozala/subscriptions","organizations_url":"https://api.github.com/users/Gozala/orgs","repos_url":"https://api.github.com/users/Gozala/repos","events_url":"https://api.github.com/users/Gozala/events{/privacy}","received_events_url":"https://api.github.com/users/Gozala/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-10-22T23:19:53Z","updated_at":"2015-10-26T22:49:24Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mozilla/browser.html/pulls/681","html_url":"https://github.com/mozilla/browser.html/pull/681","diff_url":"https://github.com/mozilla/browser.html/pull/681.diff","patch_url":"https://github.com/mozilla/browser.html/pull/681.patch"},"body":"# Do not land yet!\r\n\r\n### High level overview of the changes\r\n\r\n- [livereactload][] is by [browserify-hmr][] for couple of reasons:\r\n  1. [livereactload][] was very react specific doing bunch of code transforms to handle hot-swap react components and failed to do it properly for non-react components.\r\n  2. [livereactload][] is a port of [react-hot-loader][] that [got deprecated](https://github.com/gaearon/react-hot-loader#deprecation-notice) in favor [webpack-hmr][] + [react transform][], which is more or less what we do here although we use [browserify-hmr][] instead (since we're using browserify) and [hotify][] transform instead. Latem makes more sense for our architecture since we can reload whole app without monkey patching react components, also in this case we don't even use react or it's components.\r\n  3. Unlike  [livereactload][] which rebuild bundle on every change [hmr][webpack-hmr] rebundles only changes and sends those, which means changes to worker code just reloads a worker. Also given smaller chunks overall feedback loop is significantly faster.\r\n\r\n- legacy `about/dashboard` code was removed, in fact it was not used and is not specific to this pull.\r\n- In new reflex `address` is no longer an object but rather a function, there for:\r\n  1.  Method `address.receive(action)` is replaced by more simple `address(action)`.\r\n   2. Method `address.forward(annotate)` is gone, instead reflex provides `forward(address, annotate)` function that does exactly the same thing. Although since addresses are now functions `forward` also replaced  `address.pass(annotate)` with `forward(address, annotate)`.\r\n   3. Method `address.pass(annotate, arg1, arg2)` is gone. It was rarely used anyway, if you need it just use `forward(address, partial(annotate, arg1, arg2))` or `forward(address, action => annotate(arg1, arg2, action))`\r\n   4. Method `address.send(action)` is also gone. Again it was rarely used and did not really offered much. In most cases you better off defer action instantiation until event happens, still if you have a use case for `.send` you achieve same with `partial(address, action)`.\r\n\r\n  > Note: Above mentioned `partial` is defined in `lang/functional` module.\r\n  \r\n \r\n- Function previously called `render` was renamed to `thunk` to better match it's intent. `thunk(cacheKey, view, ...args)` creates lazy computation for `view(...args)` that is only re-computed if `...args` have changed from previous call, otherwise result of previous computation is re-used. `cacheKey` is used to identify computation as same `view` function can be used over the collection of items & there for cause unnecessary cache invalidations.\r\n- In new reflex when you create a node, last optional argument is it's children. Unlike before children must be an array (previously non-array was treated as array of that element).\r\n- In new reflex when you create a node, last optional argument must be an array. Iterators used to work before but were unintentionally broken gozala/reflex#37 because virtual-dom did not supported them. I will fix that but for now we just spread lists into arrays.\r\n\r\n- Even though reflex makes render drivers pluggable we still have some exotic use cases that have not being abstracted. There for for now we have `driver` directory that provides driver specific implementations for exotic things. All of those in case of virtual-dom can be / are implemented using [hooks][] & in a future I'd like to add [hooks][] support to react driver gozala/reflex-react-driver#2 In the meantime:\r\n\r\n  1. Non stantard attributes used to be a nightmare to deal with in react. With virtual-dom it's pretty easy: `html.iframe({ id: id, src: location, attributes: { remote: true, mozbrowser: true })` This does not yet works in react driver but intend to fix that gozala/reflex-react-driver#1\r\n\r\n  2. Non standard events used to be extremely difficult to deal with react. With virtual-dom it's no brainer using [hooks][] \\o/. Now driver exports `on(address, decoder, options, getTarget)` function where all args are optional except an `address`. This can be used to set any non standard listener: `{onMozBrowserLocationChange: on(address, eventToAction)}` creates an event listener for `mozbrowserlocationchange` event where event is decoded to an action with `eventToAction` and then send to an `address`. If `decoder` is omitted `undefined` is send to address this comes handy for with forward: `on(forward(address, Close))`. In some cases event names are not all lower case though for such events it's possible to use same `on` API but property name should not start with `on` in which case property name will be used as an even type: `({MozSwipeGesture: on(address, readGesture)})` registers event listener for `MozSwipeGesture` event.\r\n\r\n  3. An odd `animation(node, onNextFrame, isAnimating)` is replaced with another event like hook: `({onFrame: onAnimationFrame(address, decoder)})`. Similar to `on` although property name is irrelevant. address will be passed action created as `decoder(timeStamp)` as long such property is set. **Note:** this is temporary though animations will be dealt with effects instead, but that requires more refactoring.\r\n\r\n  4. meta properties like `isVisible`, `isFocused` etc... were treated specially in React. This handled by [hooks][] in virtual-dom driver case. now we have `({isFocused: focus(state.isFocused), isVisible: visibility(state.isVisible), ..})`\r\n\r\n- It turns out react and virtual-dom deal with style slightly differently. For example `{height: 50}` is interpreted by react as `height: 50px` while in case of virtual-dom it is interpreted as is meaning style rule is invalid. There for all styles should be written with units: `{height: '50px'}` which works in both rendering drivers and is kind how you do it in css anyhow.\r\n\r\n- Sending a `null` to an address used to be a noop. Now `null` is actually send (That's because addresses are simple functions now) which fails some of the code since `null` actions are not expected. In those cases I used `{}` as a workaround but should probably have special action type for that.\r\n\r\n### Todo:\r\n\r\n- [x] Loading a new tab causes frame drop and runs for more than `150ms` that means something is either wrong with timing or something is going wrong there. Unfortunately profiler seems broken in devtools but I'll investigate what is going on there.\r\n- [ ] Rewrite animations with effects\r\n- [ ] Replace IO services with effects\r\n- [ ] Swiping up or down seems to close tab, likely swipe events somehow reach cards beneath the iframe. Need to fix this.\r\n- [ ] Escape in location bar does not seem to return focus to the iframe.\r\n\r\n  P.S.: The way we used to sample was less precise than now, it could be that creating new iframes used to be always slow we just did not captured that in samples. \r\n\r\n[livereactload]:https://github.com/milankinen/livereactload\r\n[browserify-hmr]:https://github.com/AgentME/browserify-hmr\r\n[react-hot-loader]:https://gaearon.github.io/react-hot-loader/\r\n[webpack-hmr]:https://webpack.github.io/docs/hot-module-replacement.html\r\n[react-transform]:https://github.com/gaearon/react-transform-boilerplate\r\n[hotify]:https://github.com/gozala/browserify-hotify\r\n[hooks]:https://github.com/Matt-Esch/virtual-dom/blob/master/docs/hooks.md"},"comment":{"url":"https://api.github.com/repos/mozilla/browser.html/issues/comments/151306940","html_url":"https://github.com/mozilla/browser.html/pull/681#issuecomment-151306940","issue_url":"https://api.github.com/repos/mozilla/browser.html/issues/681","id":151306940,"user":{"login":"Gozala","id":21236,"avatar_url":"https://avatars.githubusercontent.com/u/21236?v=3","gravatar_id":"","url":"https://api.github.com/users/Gozala","html_url":"https://github.com/Gozala","followers_url":"https://api.github.com/users/Gozala/followers","following_url":"https://api.github.com/users/Gozala/following{/other_user}","gists_url":"https://api.github.com/users/Gozala/gists{/gist_id}","starred_url":"https://api.github.com/users/Gozala/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Gozala/subscriptions","organizations_url":"https://api.github.com/users/Gozala/orgs","repos_url":"https://api.github.com/users/Gozala/repos","events_url":"https://api.github.com/users/Gozala/events{/privacy}","received_events_url":"https://api.github.com/users/Gozala/received_events","type":"User","site_admin":false},"created_at":"2015-10-26T22:49:24Z","updated_at":"2015-10-26T22:49:24Z","body":"> Loading a new tab causes frame drop and runs for more than 150ms that means something is either wrong with timing or something is going wrong there. Unfortunately profiler seems broken in devtools but I'll investigate what is going on there.\r\n\r\nOk I have figured out what is going on there posting an image that illustrates:\r\n\r\n<img width=\"1000\" alt=\"screen shot 2015-10-26 at 3 47 57 pm\" src=\"https://cloud.githubusercontent.com/assets/21236/10744677/fa2ac154-7bf8-11e5-8496-91fcd57b2cc4.png\">\r\n\r\nTurns out it's gecko takes 200+ms to insert a mozbrowser iframe into document, which does explains while we get frame drop warning."}}