{"action":"created","issue":{"url":"https://api.github.com/repos/hammerlab/pileup.js/issues/205","labels_url":"https://api.github.com/repos/hammerlab/pileup.js/issues/205/labels{/name}","comments_url":"https://api.github.com/repos/hammerlab/pileup.js/issues/205/comments","events_url":"https://api.github.com/repos/hammerlab/pileup.js/issues/205/events","html_url":"https://github.com/hammerlab/pileup.js/pull/205","id":91093576,"number":205,"title":"Refactor code for better compatibility with CommonJS, Flow, Browserify and Watchify","user":{"login":"armish","id":7809,"avatar_url":"https://avatars.githubusercontent.com/u/7809?v=3","gravatar_id":"","url":"https://api.github.com/users/armish","html_url":"https://github.com/armish","followers_url":"https://api.github.com/users/armish/followers","following_url":"https://api.github.com/users/armish/following{/other_user}","gists_url":"https://api.github.com/users/armish/gists{/gist_id}","starred_url":"https://api.github.com/users/armish/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/armish/subscriptions","organizations_url":"https://api.github.com/users/armish/orgs","repos_url":"https://api.github.com/users/armish/repos","events_url":"https://api.github.com/users/armish/events{/privacy}","received_events_url":"https://api.github.com/users/armish/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"danvk","id":98301,"avatar_url":"https://avatars.githubusercontent.com/u/98301?v=3","gravatar_id":"","url":"https://api.github.com/users/danvk","html_url":"https://github.com/danvk","followers_url":"https://api.github.com/users/danvk/followers","following_url":"https://api.github.com/users/danvk/following{/other_user}","gists_url":"https://api.github.com/users/danvk/gists{/gist_id}","starred_url":"https://api.github.com/users/danvk/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/danvk/subscriptions","organizations_url":"https://api.github.com/users/danvk/orgs","repos_url":"https://api.github.com/users/danvk/repos","events_url":"https://api.github.com/users/danvk/events{/privacy}","received_events_url":"https://api.github.com/users/danvk/received_events","type":"User","site_admin":false},"milestone":null,"comments":1,"created_at":"2015-06-25T22:31:07Z","updated_at":"2015-06-26T15:55:44Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/hammerlab/pileup.js/pulls/205","html_url":"https://github.com/hammerlab/pileup.js/pull/205","diff_url":"https://github.com/hammerlab/pileup.js/pull/205.diff","patch_url":"https://github.com/hammerlab/pileup.js/pull/205.patch"},"body":"These changes \r\n\r\n- collect the application and test code files under a unified directory: `src/{main, test}`\r\n- add steps that, by default, **jstransform** and **browserify** the code from `src` into `build` (issue #188)\r\n- modify `package.json` so that the **main** and **browser** fields now point to plain JS-code under `build`\r\n- drops aliasing within **grunt-browserify** so that both watched tests and flow checks run smoothly (issue #200 and PR #191)\r\n- reverts the hacks introduced by PR #204)\n\n<!-- Reviewable:start -->\n[<img src=\"https://reviewable.io/review_button.png\" height=40 alt=\"Review on Reviewable\"/>](https://reviewable.io/reviews/hammerlab/pileup.js/205)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/hammerlab/pileup.js/issues/comments/115739408","html_url":"https://github.com/hammerlab/pileup.js/pull/205#issuecomment-115739408","issue_url":"https://api.github.com/repos/hammerlab/pileup.js/issues/205","id":115739408,"user":{"login":"danvk","id":98301,"avatar_url":"https://avatars.githubusercontent.com/u/98301?v=3","gravatar_id":"","url":"https://api.github.com/users/danvk","html_url":"https://github.com/danvk","followers_url":"https://api.github.com/users/danvk/followers","following_url":"https://api.github.com/users/danvk/following{/other_user}","gists_url":"https://api.github.com/users/danvk/gists{/gist_id}","starred_url":"https://api.github.com/users/danvk/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/danvk/subscriptions","organizations_url":"https://api.github.com/users/danvk/orgs","repos_url":"https://api.github.com/users/danvk/repos","events_url":"https://api.github.com/users/danvk/events{/privacy}","received_events_url":"https://api.github.com/users/danvk/received_events","type":"User","site_admin":false},"created_at":"2015-06-26T15:55:43Z","updated_at":"2015-06-26T15:55:43Z","body":"Thanks for making this huge change! It's going to simplify distribution immensely.\n\nI've left a few scattered comments, but my main bit of feedback is that we should either ditch Grunt in favor of shell scripts/npm scripts or use it for everything. I don't want two build systems. I'm fine going either way. Arguments in favor of grunt are that all the commands are centralized and JavaScript is a better language than shell. Arguments in favor of shell scripts are that we can ditch the `grunt-*` wrappers around everything we want to use.\n\n---\n\nReviewed 9 of 185 files at r1.\nReview status: 9 of 185 files reviewed at latest revision, 16 unresolved discussions, all commit checks successful.\n\n---\n\n<sup>**[.flowconfig, line 9 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskNj5dmOE9-qL3A86i)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/.flowconfig#L9)):</sup>\nI believe this is superfluous -- everything under the project root will be included by default. See http://flowtype.org/docs/advanced-configuration.html#example\n\n---\n\n<sup>**[examples/playground-complete.html, line 19 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskRD0X3gis4juJ_lDt)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/examples/playground-complete.html#L19)):</sup>\nAny reason for the rename? Since the name of the product is \"pileup.js\", `pileup.js` seems like a good name for the JS bundle :)\n\nI initially read this as \"pileup browser\", rather than \"pileup built for the web browser\".\n\n---\n\n<sup>**[examples/playground.js, line 1 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskROqrxXOPIEcVB2Gh)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/examples/playground.js#L1)):</sup>\nThis has always puzzled me—when you source `pileup.min.js`, does it define `require` globally?  That is somewhat surprising behavior.\n\n---\n\n<sup>**[examples/playground.js, line 29 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskRlqaRCxZOiqJC9UQ)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/examples/playground.js#L29)):</sup>\nWhy this reorganization? (`/test/data`→`/test-data`)\n\n---\n\n<sup>**[package.json, line 20 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskXu7LYJl9josHUAEq)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/package.json#L20)):</sup>\nLooking at other packages, it seems like `dist` is a bit more standard (jQuery and react use it, for example). The other common pattern is to put all the build artifacts in a top-level directory (like Q and D3).\n\nRenaming to `dist` is probably easier. What do you think? I know I was the one who suggested `build`—sorry!\n\n---\n\n<sup>**[package.json, line 21 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskXdCQXifjNmPOB_qJ)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/package.json#L21)):</sup>\nI don't think we want to distribute `src` any more: users will only be interested in the plain-old-JavaScript version.\n\n---\n\n<sup>**[package.json, line 29 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskSUEhyR7muc8Lwy6v)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/package.json#L29)):</sup>\nDo you want to abandon grunt?\n\n---\n\n<sup>**[README.md, line 13 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskSmSFapL128ylrMEu)** ([raw file](https://github.com/hammerlab/pileup.js/blob/ebfb445487da7cad593fa85938d7d02a01cf6bc5/README.md#L13)):</sup>\nThis will only work if you have `node_modules/.bin` on your `$PATH`. Maybe worth calling that out, or leaving as-is.\n\n---\n\n<sup>**[README.md, line 14 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskT43dEMXRhSJ2paJk)** ([raw file](https://github.com/hammerlab/pileup.js/blob/ebfb445487da7cad593fa85938d7d02a01cf6bc5/README.md#L14)):</sup>\nYou don't need to build anything?\n\nWe should reorganize these sections. The \"Usage\" section is more interesting to users and should go on top. The \"Quickstart\" section is of interest to developers working on pileup and should be renamed something like \"Development Setup\".\n\n---\n\n<sup>**[README.md, line 96 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskU1TGPUA96hmM_WS2)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/README.md#L96)):</sup>\nAre both of these commands necessary?\n\n---\n\n<sup>**[README.md, line 98 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskTsC4wolNxMmRMGPL)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/README.md#L98)):</sup>\nThe quotes aren't necessary here.\n\n---\n\n<sup>**[README.md, line 107 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskTXL_1BjFR3isYMjj)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/README.md#L107)):</sup>\nIt's actually way better to run `flow status` for type checking. The `grunt flow` command starts up a new Flow process and type checks everything from scratch, which takes 10-20 seconds. `flow status` starts a background server which takes 10-20 seconds the first time, but ~100ms thereafter.\n\n---\n\n<sup>**[scripts/jstransform.sh, line 3 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskUFS0EcpYxUbgWT_i)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/scripts/jstransform.sh#L3)):</sup>\nyou don't need to `export` this variable to the environment—you only use it in this script, rather than in child processes.\n\n---\n\n<sup>**[scripts/jstransform.sh, line 3 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskUXN6122XMIoZPuOL)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/scripts/jstransform.sh#L3)):</sup>\nChange `${JSTRANSFORM_OPTS}` to `$@` and move it to the end. That way you can invoke this script as `./scripts/jstransform.sh --watch`, rather than dealing with environment variables. It will also let you override default options, e.g. if you wanted to do `./scripts/jstransform.sh --noharmony`:\n\n```bash\nJSTRANSFORM_OPTS=\"--react --harmony --strip-types --non-strict-es6module --source-map-inline $@\"\n```\n\n---\n\n<sup>**[scripts/jstransform.sh, line 4 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskUCgz1WweubM2Imgq)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/scripts/jstransform.sh#L4)):</sup>\nremove debug code\n\n---\n\n<sup>**[scripts/jstransform.sh, line 5 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pileup.js/205#-JskV3MlRrgM-r0wjRUQ)** ([raw file](https://github.com/hammerlab/pileup.js/blob/bb685cc6b09a50162f054ca7bdffe7e787399dc3/scripts/jstransform.sh#L5)):</sup>\nMove this comment up to the top (after the shebang line). It describes what the entire script does.\n\n---\n\n\nComments from the [review on Reviewable.io](https://reviewable.io:443/reviews/hammerlab/pileup.js/205)\n<!-- Sent from Reviewable.io -->\n"}}