{"action":"created","issue":{"url":"https://api.github.com/repos/substack/node-browserify/issues/1262","labels_url":"https://api.github.com/repos/substack/node-browserify/issues/1262/labels{/name}","comments_url":"https://api.github.com/repos/substack/node-browserify/issues/1262/comments","events_url":"https://api.github.com/repos/substack/node-browserify/issues/1262/events","html_url":"https://github.com/substack/node-browserify/pull/1262","id":75750287,"number":1262,"title":"Fix handling of `transform` props in b.require()","user":{"login":"jmm","id":129203,"avatar_url":"https://avatars.githubusercontent.com/u/129203?v=3","gravatar_id":"","url":"https://api.github.com/users/jmm","html_url":"https://github.com/jmm","followers_url":"https://api.github.com/users/jmm/followers","following_url":"https://api.github.com/users/jmm/following{/other_user}","gists_url":"https://api.github.com/users/jmm/gists{/gist_id}","starred_url":"https://api.github.com/users/jmm/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jmm/subscriptions","organizations_url":"https://api.github.com/users/jmm/orgs","repos_url":"https://api.github.com/users/jmm/repos","events_url":"https://api.github.com/users/jmm/events{/privacy}","received_events_url":"https://api.github.com/users/jmm/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":6,"created_at":"2015-05-12T21:45:22Z","updated_at":"2015-05-15T04:06:38Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/substack/node-browserify/pulls/1262","html_url":"https://github.com/substack/node-browserify/pull/1262","diff_url":"https://github.com/substack/node-browserify/pull/1262.diff","patch_url":"https://github.com/substack/node-browserify/pull/1262.patch"},"body":"Supersedes #1057.\r\n\r\n<hr />\r\nEDIT 2015-05-13 4:52 eastern\r\nSee updated commits (discussed below):\r\n\r\nHere's an [updated branch](https://github.com/jmm/node-browserify/tree/b-require-transform2):\r\n\r\nPart that corresponds to original PR branch:\r\nhttps://github.com/substack/node-browserify/compare/master...jmm:bbb1ab8\r\n\r\nGeneral refactoring on top of that:\r\nhttps://github.com/jmm/node-browserify/compare/bbb1ab8...b-require-transform2\r\n<hr />"},"comment":{"url":"https://api.github.com/repos/substack/node-browserify/issues/comments/102248233","html_url":"https://github.com/substack/node-browserify/pull/1262#issuecomment-102248233","issue_url":"https://api.github.com/repos/substack/node-browserify/issues/1262","id":102248233,"user":{"login":"zertosh","id":830952,"avatar_url":"https://avatars.githubusercontent.com/u/830952?v=3","gravatar_id":"","url":"https://api.github.com/users/zertosh","html_url":"https://github.com/zertosh","followers_url":"https://api.github.com/users/zertosh/followers","following_url":"https://api.github.com/users/zertosh/following{/other_user}","gists_url":"https://api.github.com/users/zertosh/gists{/gist_id}","starred_url":"https://api.github.com/users/zertosh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/zertosh/subscriptions","organizations_url":"https://api.github.com/users/zertosh/orgs","repos_url":"https://api.github.com/users/zertosh/repos","events_url":"https://api.github.com/users/zertosh/events{/privacy}","received_events_url":"https://api.github.com/users/zertosh/received_events","type":"User","site_admin":false},"created_at":"2015-05-15T04:06:37Z","updated_at":"2015-05-15T04:06:37Z","body":"> it's a more dramatic refactoring (not that that's a bad thing)\r\n\r\nHeh, I think it's actually the simplest solution https://gist.github.com/zertosh/e66a3e96afc45228f02b\r\n\r\n> The logic in that method is very convoluted, almost entirely lacking comments, and has questionable test coverage. Aside from that, you'd have to do something (maybe a private method and a public method) to prevent users from populating order, right?\r\n\r\n\"Questionable test coverage\" lol - so true. All of the `expose` logic above it doesn't make sense for streams:\r\n\r\n```js\r\n// never true\r\nif (file === expose && /^[\\.]/.test(expose)) {\r\n    expose = '/' + path.relative(basedir, expose);\r\n}\r\n// if this is true.....\r\nif (expose === undefined && this._options.exposeAll) {\r\n    expose = true;\r\n}\r\n// then this will throw because `path.relative` only\r\n// takes string arguments - file -> stream\r\nif (expose === true) {\r\n    expose = '/' + path.relative(basedir, file);\r\n}\r\n```\r\n\r\nI not sure about making public/private methods. If you mess with `order` then you deserve what you've got coming to you hehe. Not a huge concern though, I think the streams logic here is the for the CLI - it passes `process.stdin` as the file stream. BUT once the `row` props are figured out, something like order should be renamed to `_order`.\r\n\r\n> My philosophy is generally to very much emphasize non-duplication. These tests are very closely related variations on a theme, differing only by a couple of parameters.\r\n\r\nDon't get me wrong, I'm not in favor of code duplication - except in tests :P Tests have to be the easiest possible to understand, with very little surface area for errors in themselves. Introducing abstractions or syntax works against that. But I won't block you over testing philosophy.\r\n\r\n> I was under the impression that the way I authored it would avoid waiting for timeout\r\n\r\nYup. You're right. I just verified it :+1:\r\n"}}