{"action":"created","comment":{"url":"https://api.github.com/repos/nodejs/node-eps/pulls/comments/115117139","pull_request_review_id":36640992,"id":115117139,"diff_hunk":"@@ -0,0 +1,192 @@\n+| Title  | Staged require() with lifecycle hooks |\n+|--------|---------------------------------------|\n+| Author | @qard                                 |\n+| Status | DRAFT                                 |\n+| Date   | 2017-05-05                            |\n+\n+# Proposal\n+\n+There are two major purposes to this proposal:\n+- To provide module loading lifecycle hooks\n+- To split resolving, loading and compilation concerns\n+\n+# Motivations\n+\n+Currently some very popular userland modules depend heavily on monkey-patching\n+to provide their functionality. This can be unsafe, inflexible and damaging to\n+performance.\n+\n+Some examples include:\n+\n+- Tracing, debugging and monitoring\n+- Code coverage\n+- Mocking\n+- Transpiling\n+\n+### Safety\n+\n+Monkey-patching can be unsafe for many reasons. Sometimes code the behaviour\n+of code changes based on parameter length of input functions--express is a good\n+example of that. Sometimes patches can interfere with getter/setter behaviour.\n+It can be difficult to differentiate between async callbacks that need code to\n+maintain barrier linkage and sync callbacks which do not. Some interfaces\n+accept multiple function arguments and dynamically identifying which is a\n+callback that needs to be instrumented can be unreliable. Any form of queueing,\n+even simply putting a callback in an array to be called elsewhere, will break\n+continuation linking and requires manual intervention to maintain that linkage\n+which is not always possible for dynamic instrumentation.\n+\n+Most APM providers try/catch constantly because the existing solutions for\n+tracking continuation lose context regularly. We've done the best we can with\n+monkey-patching, but we've hit a wall and have been pushing for things like\n+`async-hooks` to improve the situation. However, `async-hooks` only solves part\n+of the problem--linking the callsite to the callback--but context on what was\n+happening still requires monkey-patching, and there are plenty of situations\n+where context-loss is not the hard part.\n+\n+### Performance\n+\n+Monkey-patching is prone to performance issues. The vast majority requires\n+wrapping things in closures. More complex instrumentation can often trigger\n+deoptimization. Sometimes heavy runtime checking is required with constant\n+dynamic function generation and reassignment, which can put a lot of undue\n+stress on the optimizer and garbage collector.\n+\n+If, for example, one could intercept the module loader and apply an AST\n+transform before compilation, there's a great deal that could be gained.\n+Most Run-time checking would simply disappear. Most closures would disappear.\n+Context loss would disappear because the transformer could take advantage of\n+lexical scope to describe linkage directly at callsites.\n+\n+### Flexibility\n+\n+Many things are just not possible to any reasonable capacity relying purely on\n+monkey-patching. Often things which could provide valuable coverage or tracing\n+insight are simply not exposed.\n+\n+An example of a difficult thing to apply tracing hooks to is the timing from\n+when a socket first enters JS in `net.js` to when it switches to a TLS socket.\n+One could listen for the `secureConnection` event, but there are several\n+failure paths between the `TLSSocket` constructor and the event handler being\n+triggered, which could result in context loss. One might also think to patch\n+`TLSSocket.prototype._init`, however that will be called by both servers and\n+clients, which are difficult to differentiate from in this case. To correctly\n+identify and correlate a net socket to a corresponding TLS upgrade takes\n+extensive monkey-patching with a lot of run-time checks in a major hot-path.\n+With an AST transform, a single line of code could be inserted\n+[before the `TLSSocket` construction][1] in the `Server` constructor.\n+\n+### ES Modules\n+\n+A big issue on the horizon is that, if ES Modules are adopted, modules become\n+immutable at build time. This means that the current monkey-patching approach\n+will not work anymore. TC39 has already suggested using AST transformation\n+to deal with this issue.\n+\n+Another concern with ES Modules is the difference in loader behaviour. This is\n+where splitting resolution and loading concerns from compilation is valuable.\n+\n+# Implementation\n+\n+The idea is to go for something similar to the existing `Module._extensions`\n+interface, but with three stages: resolving, loading and compiling.\n+\n+Each stage would interact with a `ModuleRequest` object which expresses the\n+lifetime of a module request through the `resolve`, `load` and `compile` stages.\n+Initially, it would only include the `parent` and `inputPath` fields. Other\n+fields would be populated in the different lifecycle stages.\n+\n+### `ModuleRequest`\n+\n+##### `moduleRequest.parent`\n+\n+The parent module which initiated the `require()` request.\n+\n+##### `moduleRequest.inputPath`\n+\n+The input string to the `require()` call or `import` statement which initiated\n+this module request.\n+\n+##### `moduleRequest.resolvedPath`\n+\n+The fully resolved file path, as determined by the `require.resolvers` handler.\n+This would be added to the module request object during the `resolver` stage.\n+\n+##### `moduleRequest.contents`\n+\n+The content string (or buffer?) of the module. This field is populated during\n+the `loader` stage.\n+\n+##### `moduleRequest.module`\n+\n+The final module object, built by the `compiler` stage.\n+\n+### `require.resolvers`\n+\n+It is the responsibility of `require.resolvers` extension handlers to receive\n+the in-process `Module` object and use the metadata on it to figure out how to\n+resolve a load. For `require()` relative location loading, it'd simply look at\n+the `parent` and resolve the input string of the `require()` call relative to\n+the `parent` module.\n+\n+This would roughly correspond to the current `Module._resolveLookupPaths`\n+implementation, but it should instead receive the module request object which\n+it will modify in-place.\n+\n+```js\n+const mockedPaths = {\n+  'foo.js': path.join(__dirname, 'mocked-foo.js')\n+}\n+\n+const originalResolver = require.resolvers['require']\n+require.resolvers['require'] = function customResolver(request) {\n+  if (Object.keys(mockedPaths).includes(request.inputPath)) {\n+    return mockedPaths[request.inputPath]\n+  }\n+  originalResolver(request)\n+}\n+```\n+\n+By defining named behaviour specific to `require()`, we open the door for\n+supporting the different behaviour of `import` in the future.\n+\n+### `require.loaders`\n+\n+The `require.loaders` extension handlers receive the `Module` object after the\n+`resolver` stage and use the resolved path to attempt to load the contents of\n+the module file.\n+\n+```js\n+const originalLoader = require.loaders['.js']\n+require.loaders['.js'] = function customLoader(request) {\n+  // This populates the `contents` field\n+  originalLoader(request)\n+\n+  request.contents = request.contents.replace(\n+    /module\\.exports\\s+\\=/,\n+    // Don't forget that `exports` object now!\n+    'exports = module.exports ='\n+  )\n+}\n+```\n+\n+### `require.compilers`\n+\n+The `require.compilers` extension handlers receive the `Module` object after the\n+`loader` stage and pass the `contents` into a compiler function to build the\n+final module object.\n+\n+```js\n+require.compilers['.yaml'] = function yamlCompiler(request) {\n+  request.module.exports = yaml.parse(request.contents)\n+}\n+```","path":"XXX-staged-require-with-lifecycle-hooks.md","position":183,"original_position":183,"commit_id":"375b0dc55ff7115da8a66849e3864fd51ec411b6","original_commit_id":"375b0dc55ff7115da8a66849e3864fd51ec411b6","user":{"login":"sam-github","id":17607,"avatar_url":"https://avatars1.githubusercontent.com/u/17607?v=3","gravatar_id":"","url":"https://api.github.com/users/sam-github","html_url":"https://github.com/sam-github","followers_url":"https://api.github.com/users/sam-github/followers","following_url":"https://api.github.com/users/sam-github/following{/other_user}","gists_url":"https://api.github.com/users/sam-github/gists{/gist_id}","starred_url":"https://api.github.com/users/sam-github/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sam-github/subscriptions","organizations_url":"https://api.github.com/users/sam-github/orgs","repos_url":"https://api.github.com/users/sam-github/repos","events_url":"https://api.github.com/users/sam-github/events{/privacy}","received_events_url":"https://api.github.com/users/sam-github/received_events","type":"User","site_admin":false},"body":"might be possible to optimize inside module. If the array has only one member, the function used to iterate the array can be replaced directly with the only function in the array. For arrays longer than one, we might even be able to pre-compose a function out of the members of the array (basically, cacheing them to avoid repeated iteration).","created_at":"2017-05-06T09:46:50Z","updated_at":"2017-05-06T09:46:50Z","html_url":"https://github.com/nodejs/node-eps/pull/56#discussion_r115117139","pull_request_url":"https://api.github.com/repos/nodejs/node-eps/pulls/56","_links":{"self":{"href":"https://api.github.com/repos/nodejs/node-eps/pulls/comments/115117139"},"html":{"href":"https://github.com/nodejs/node-eps/pull/56#discussion_r115117139"},"pull_request":{"href":"https://api.github.com/repos/nodejs/node-eps/pulls/56"}}},"pull_request":{"url":"https://api.github.com/repos/nodejs/node-eps/pulls/56","id":119287335,"html_url":"https://github.com/nodejs/node-eps/pull/56","diff_url":"https://github.com/nodejs/node-eps/pull/56.diff","patch_url":"https://github.com/nodejs/node-eps/pull/56.patch","issue_url":"https://api.github.com/repos/nodejs/node-eps/issues/56","number":56,"state":"open","locked":false,"title":"proposal: Staged require() with lifecycle hooks","user":{"login":"Qard","id":205482,"avatar_url":"https://avatars3.githubusercontent.com/u/205482?v=3","gravatar_id":"","url":"https://api.github.com/users/Qard","html_url":"https://github.com/Qard","followers_url":"https://api.github.com/users/Qard/followers","following_url":"https://api.github.com/users/Qard/following{/other_user}","gists_url":"https://api.github.com/users/Qard/gists{/gist_id}","starred_url":"https://api.github.com/users/Qard/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Qard/subscriptions","organizations_url":"https://api.github.com/users/Qard/orgs","repos_url":"https://api.github.com/users/Qard/repos","events_url":"https://api.github.com/users/Qard/events{/privacy}","received_events_url":"https://api.github.com/users/Qard/received_events","type":"User","site_admin":false},"body":"I'm still working on this, but I think it's time to get some more eyes on it.\r\n\r\nThis is the EPS continuation of my work in https://github.com/nodejs/node/pull/12349 to attempt to enable an AST transformation pipeline in Node.js core. There's definitely some stuff missing still, and I'll add more over the weekend, but this is where I'm at today. 😸 \r\n\r\n@nodejs/diagnostics ","created_at":"2017-05-06T05:04:14Z","updated_at":"2017-05-06T09:46:50Z","closed_at":null,"merged_at":null,"merge_commit_sha":"95bee9c67d54e5d604e0adf11c1abc93ad194d21","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/nodejs/node-eps/pulls/56/commits","review_comments_url":"https://api.github.com/repos/nodejs/node-eps/pulls/56/comments","review_comment_url":"https://api.github.com/repos/nodejs/node-eps/pulls/comments{/number}","comments_url":"https://api.github.com/repos/nodejs/node-eps/issues/56/comments","statuses_url":"https://api.github.com/repos/nodejs/node-eps/statuses/375b0dc55ff7115da8a66849e3864fd51ec411b6","head":{"label":"Qard:staged-require","ref":"staged-require","sha":"375b0dc55ff7115da8a66849e3864fd51ec411b6","user":{"login":"Qard","id":205482,"avatar_url":"https://avatars3.githubusercontent.com/u/205482?v=3","gravatar_id":"","url":"https://api.github.com/users/Qard","html_url":"https://github.com/Qard","followers_url":"https://api.github.com/users/Qard/followers","following_url":"https://api.github.com/users/Qard/following{/other_user}","gists_url":"https://api.github.com/users/Qard/gists{/gist_id}","starred_url":"https://api.github.com/users/Qard/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Qard/subscriptions","organizations_url":"https://api.github.com/users/Qard/orgs","repos_url":"https://api.github.com/users/Qard/repos","events_url":"https://api.github.com/users/Qard/events{/privacy}","received_events_url":"https://api.github.com/users/Qard/received_events","type":"User","site_admin":false},"repo":{"id":90089679,"name":"node-eps","full_name":"Qard/node-eps","owner":{"login":"Qard","id":205482,"avatar_url":"https://avatars3.githubusercontent.com/u/205482?v=3","gravatar_id":"","url":"https://api.github.com/users/Qard","html_url":"https://github.com/Qard","followers_url":"https://api.github.com/users/Qard/followers","following_url":"https://api.github.com/users/Qard/following{/other_user}","gists_url":"https://api.github.com/users/Qard/gists{/gist_id}","starred_url":"https://api.github.com/users/Qard/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Qard/subscriptions","organizations_url":"https://api.github.com/users/Qard/orgs","repos_url":"https://api.github.com/users/Qard/repos","events_url":"https://api.github.com/users/Qard/events{/privacy}","received_events_url":"https://api.github.com/users/Qard/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/Qard/node-eps","description":"Node.js Enhancement Proposals for discussion on future API additions/changes to Node core","fork":true,"url":"https://api.github.com/repos/Qard/node-eps","forks_url":"https://api.github.com/repos/Qard/node-eps/forks","keys_url":"https://api.github.com/repos/Qard/node-eps/keys{/key_id}","collaborators_url":"https://api.github.com/repos/Qard/node-eps/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/Qard/node-eps/teams","hooks_url":"https://api.github.com/repos/Qard/node-eps/hooks","issue_events_url":"https://api.github.com/repos/Qard/node-eps/issues/events{/number}","events_url":"https://api.github.com/repos/Qard/node-eps/events","assignees_url":"https://api.github.com/repos/Qard/node-eps/assignees{/user}","branches_url":"https://api.github.com/repos/Qard/node-eps/branches{/branch}","tags_url":"https://api.github.com/repos/Qard/node-eps/tags","blobs_url":"https://api.github.com/repos/Qard/node-eps/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/Qard/node-eps/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/Qard/node-eps/git/refs{/sha}","trees_url":"https://api.github.com/repos/Qard/node-eps/git/trees{/sha}","statuses_url":"https://api.github.com/repos/Qard/node-eps/statuses/{sha}","languages_url":"https://api.github.com/repos/Qard/node-eps/languages","stargazers_url":"https://api.github.com/repos/Qard/node-eps/stargazers","contributors_url":"https://api.github.com/repos/Qard/node-eps/contributors","subscribers_url":"https://api.github.com/repos/Qard/node-eps/subscribers","subscription_url":"https://api.github.com/repos/Qard/node-eps/subscription","commits_url":"https://api.github.com/repos/Qard/node-eps/commits{/sha}","git_commits_url":"https://api.github.com/repos/Qard/node-eps/git/commits{/sha}","comments_url":"https://api.github.com/repos/Qard/node-eps/comments{/number}","issue_comment_url":"https://api.github.com/repos/Qard/node-eps/issues/comments{/number}","contents_url":"https://api.github.com/repos/Qard/node-eps/contents/{+path}","compare_url":"https://api.github.com/repos/Qard/node-eps/compare/{base}...{head}","merges_url":"https://api.github.com/repos/Qard/node-eps/merges","archive_url":"https://api.github.com/repos/Qard/node-eps/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/Qard/node-eps/downloads","issues_url":"https://api.github.com/repos/Qard/node-eps/issues{/number}","pulls_url":"https://api.github.com/repos/Qard/node-eps/pulls{/number}","milestones_url":"https://api.github.com/repos/Qard/node-eps/milestones{/number}","notifications_url":"https://api.github.com/repos/Qard/node-eps/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/Qard/node-eps/labels{/name}","releases_url":"https://api.github.com/repos/Qard/node-eps/releases{/id}","deployments_url":"https://api.github.com/repos/Qard/node-eps/deployments","created_at":"2017-05-03T00:02:09Z","updated_at":"2017-05-02T19:56:12Z","pushed_at":"2017-05-06T04:59:50Z","git_url":"git://github.com/Qard/node-eps.git","ssh_url":"git@github.com:Qard/node-eps.git","clone_url":"https://github.com/Qard/node-eps.git","svn_url":"https://github.com/Qard/node-eps","homepage":null,"size":55,"stargazers_count":0,"watchers_count":0,"language":null,"has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"nodejs:master","ref":"master","sha":"54b199c4eb58ff8b3ac8f273bd52af727bfc9dac","user":{"login":"nodejs","id":9950313,"avatar_url":"https://avatars0.githubusercontent.com/u/9950313?v=3","gravatar_id":"","url":"https://api.github.com/users/nodejs","html_url":"https://github.com/nodejs","followers_url":"https://api.github.com/users/nodejs/followers","following_url":"https://api.github.com/users/nodejs/following{/other_user}","gists_url":"https://api.github.com/users/nodejs/gists{/gist_id}","starred_url":"https://api.github.com/users/nodejs/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nodejs/subscriptions","organizations_url":"https://api.github.com/users/nodejs/orgs","repos_url":"https://api.github.com/users/nodejs/repos","events_url":"https://api.github.com/users/nodejs/events{/privacy}","received_events_url":"https://api.github.com/users/nodejs/received_events","type":"Organization","site_admin":false},"repo":{"id":48204104,"name":"node-eps","full_name":"nodejs/node-eps","owner":{"login":"nodejs","id":9950313,"avatar_url":"https://avatars0.githubusercontent.com/u/9950313?v=3","gravatar_id":"","url":"https://api.github.com/users/nodejs","html_url":"https://github.com/nodejs","followers_url":"https://api.github.com/users/nodejs/followers","following_url":"https://api.github.com/users/nodejs/following{/other_user}","gists_url":"https://api.github.com/users/nodejs/gists{/gist_id}","starred_url":"https://api.github.com/users/nodejs/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nodejs/subscriptions","organizations_url":"https://api.github.com/users/nodejs/orgs","repos_url":"https://api.github.com/users/nodejs/repos","events_url":"https://api.github.com/users/nodejs/events{/privacy}","received_events_url":"https://api.github.com/users/nodejs/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/nodejs/node-eps","description":"Node.js Enhancement Proposals for discussion on future API additions/changes to Node core","fork":false,"url":"https://api.github.com/repos/nodejs/node-eps","forks_url":"https://api.github.com/repos/nodejs/node-eps/forks","keys_url":"https://api.github.com/repos/nodejs/node-eps/keys{/key_id}","collaborators_url":"https://api.github.com/repos/nodejs/node-eps/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/nodejs/node-eps/teams","hooks_url":"https://api.github.com/repos/nodejs/node-eps/hooks","issue_events_url":"https://api.github.com/repos/nodejs/node-eps/issues/events{/number}","events_url":"https://api.github.com/repos/nodejs/node-eps/events","assignees_url":"https://api.github.com/repos/nodejs/node-eps/assignees{/user}","branches_url":"https://api.github.com/repos/nodejs/node-eps/branches{/branch}","tags_url":"https://api.github.com/repos/nodejs/node-eps/tags","blobs_url":"https://api.github.com/repos/nodejs/node-eps/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/nodejs/node-eps/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/nodejs/node-eps/git/refs{/sha}","trees_url":"https://api.github.com/repos/nodejs/node-eps/git/trees{/sha}","statuses_url":"https://api.github.com/repos/nodejs/node-eps/statuses/{sha}","languages_url":"https://api.github.com/repos/nodejs/node-eps/languages","stargazers_url":"https://api.github.com/repos/nodejs/node-eps/stargazers","contributors_url":"https://api.github.com/repos/nodejs/node-eps/contributors","subscribers_url":"https://api.github.com/repos/nodejs/node-eps/subscribers","subscription_url":"https://api.github.com/repos/nodejs/node-eps/subscription","commits_url":"https://api.github.com/repos/nodejs/node-eps/commits{/sha}","git_commits_url":"https://api.github.com/repos/nodejs/node-eps/git/commits{/sha}","comments_url":"https://api.github.com/repos/nodejs/node-eps/comments{/number}","issue_comment_url":"https://api.github.com/repos/nodejs/node-eps/issues/comments{/number}","contents_url":"https://api.github.com/repos/nodejs/node-eps/contents/{+path}","compare_url":"https://api.github.com/repos/nodejs/node-eps/compare/{base}...{head}","merges_url":"https://api.github.com/repos/nodejs/node-eps/merges","archive_url":"https://api.github.com/repos/nodejs/node-eps/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/nodejs/node-eps/downloads","issues_url":"https://api.github.com/repos/nodejs/node-eps/issues{/number}","pulls_url":"https://api.github.com/repos/nodejs/node-eps/pulls{/number}","milestones_url":"https://api.github.com/repos/nodejs/node-eps/milestones{/number}","notifications_url":"https://api.github.com/repos/nodejs/node-eps/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/nodejs/node-eps/labels{/name}","releases_url":"https://api.github.com/repos/nodejs/node-eps/releases{/id}","deployments_url":"https://api.github.com/repos/nodejs/node-eps/deployments","created_at":"2015-12-17T23:39:07Z","updated_at":"2017-05-04T12:18:47Z","pushed_at":"2017-05-06T05:04:15Z","git_url":"git://github.com/nodejs/node-eps.git","ssh_url":"git@github.com:nodejs/node-eps.git","clone_url":"https://github.com/nodejs/node-eps.git","svn_url":"https://github.com/nodejs/node-eps","homepage":null,"size":51,"stargazers_count":265,"watchers_count":265,"language":null,"has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":45,"mirror_url":null,"open_issues_count":21,"forks":45,"open_issues":21,"watchers":265,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/nodejs/node-eps/pulls/56"},"html":{"href":"https://github.com/nodejs/node-eps/pull/56"},"issue":{"href":"https://api.github.com/repos/nodejs/node-eps/issues/56"},"comments":{"href":"https://api.github.com/repos/nodejs/node-eps/issues/56/comments"},"review_comments":{"href":"https://api.github.com/repos/nodejs/node-eps/pulls/56/comments"},"review_comment":{"href":"https://api.github.com/repos/nodejs/node-eps/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/nodejs/node-eps/pulls/56/commits"},"statuses":{"href":"https://api.github.com/repos/nodejs/node-eps/statuses/375b0dc55ff7115da8a66849e3864fd51ec411b6"}}}}
{"actor":{"display_login":"sam-github"}}