Node.js HTTP #97
Node.js HTTP #97
Conversation
| res.end('done'); | ||
| }).listen(); | ||
|
|
||
| addAsyncTest('HTTP - node:http - client request', async function (test) { |
There was a problem hiding this comment.
There probably should be a test for:
- calling HTTP.get, and synchronously (without awaiting HTTP.get) call node:http afterwards
- do the same thing for fetch
This will ensure that isHTTPAlreadyTracked is cleaned up correctly.
There was a problem hiding this comment.
Makes sense
There was a problem hiding this comment.
Yeah, it does pollute the current context and leaks even when both are awaited... having a hard time thinking on a variable or parameter to use to prevent the duplication.
There was a problem hiding this comment.
I tried using a separate store with run and it worked fine. I think we can use Meteor.Environment on Meteor 2.
lib/hijack/http.js
Outdated
| @@ -92,3 +98,43 @@ if (Package['fetch']) { | |||
| } | |||
| }; | |||
| } | |||
|
|
|||
| const { Agent } = require('_http_agent'); | |||
There was a problem hiding this comment.
Is it possible to use the node: prefix here?
There was a problem hiding this comment.
I thought it would cause problems with older Node versions but it does work
There was a problem hiding this comment.
If we backport to Meteor 2, we wouldn't be able to use it there.
There was a problem hiding this comment.
Yes, makes sense, will check if future Node.js versions enforce the prefix and implement a workaround.
lib/hijack/http.js
Outdated
| @@ -21,6 +23,8 @@ if (Package['http']) { | |||
| return originalCall.apply(this, arguments); | |||
| } | |||
|
|
|||
| mergeEnterWith({ isHttpAlreadyTracked: true }); | |||
There was a problem hiding this comment.
Is there another option for storing isHttpAlreadyTracked? That would make it easier to backport this for Meteor 2.
Does the HTTP and fetch packages synchronously create the request? If so, we could use a variable to track it. Another option might be to wrap it with a Meteor Environment Variable.
There was a problem hiding this comment.
I tried using Meteor.EnvironmentVariable but the withValue wraps the logic into a promise and causes problems with errors being thrown inside it (uncaught error, instead of propagating upstream). Will research a better way... in the meantime I asked the guys at Meteor what they think of removing the async/await here:
There was a problem hiding this comment.
There are a number of bugs with Meteor.EnvironmentVariable in Meteor 3 at the moment. We could maybe wait on this until those get fix.
Does the HTTP and fetch packages synchronously create the request? If so, we could use a variable to track it.
Do you know if this would work?
There was a problem hiding this comment.
I could not find a way to use a variable to do that, there are simply too many layers between the calls, but using run and a new als store did the trick. The Meteor.EnvironmentVariable should work on Meteor 2 as an alternative I think.
Intercept all HTTP requests by wrapping
addRequestfrom the internal HTTP Agent (_http_agent).If the HTTP request is already tracked by the hijacks for
meteor/httpormeteor/fetchthen we defer to them by using a ALS flag.It tracks the requests namespaced as the library name
node:http.Added
remedaas an alternative tounderscoreand eliminated some unusedlodashstandalone modules I had added. We need a good library like these for more declarative and readable code, andremedalooks like a great fit.