balena-io-modules / balena-request Goto Github PK

It looks like recent releases of global-tunnel-ng don't actually handle proxies correctly, and at the very least the tests in this repo fail.

This is required for log streaming, since the request never completes.

HQ: https://github.com/resin-io/hq/pull/370/
Depends-on: https://github.com/resin-io/resin-api/issues/509

It's api_key in this module, but it should be apikey
I wonder how it works, maybe it's manually passed as part of query or never passed at all?

Confirmation that it shouldn't have an underscore: https://github.com/resin-io/resin-supervisor/blob/c4f33d55eaa5d10fd986b00c0c85ee7b810fb6f7/src/application.coffee#L306

I can fix it

It should be 'blob' | 'text' | 'json' and should have higher priority than the response content-type detection

See: https://www.flowdock.com/app/rulemotion/resin-frontend/threads/Jyw_ISIxLe17yp28RvJCVMsh-Cm
See: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API#Aborting_a_fetch

See: https://github.com/balena-io-modules/balena-request/pull/129/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R47
See: balena-io/balena-sdk#663

We're getting security warnings about handlebars, marked and uglify. None of these should actually affect this package, but we should update them anyway to guarantee that, and get rid of the warnings here.

See: https://www.flowdock.com/app/rulemotion/r-beginners/threads/fnuvWdmMTYdgY4sAitqaYvA0-f3

Maybe we could just expose a method that says installMocks that would return a properly configured fetchMock instance as well as setup all the expected things.

Or just abstract the setup code to a single submodule and expose a method setFetch.

See this thread: balena-io-modules/balena-pine#25

We currently assume that if there is a not null or undefined error property, then its an object and try to extract its text property, which results ending up with undefined error messages.
See: https://github.com/resin-io-modules/resin-request/blob/2123f5915c515994d248771b683dc6b1f3886edf/lib/utils.coffee#L104
See: https://github.com/resin-io/resin-api/blob/e6d53fe97ea91bf406415ab46dbbeac78645e7ed/src/models/hooks/index.coffee#L1171

The ponyfill module always returns the version based on XHR [https://github.com/qubyte/fetch-ponyfill/issues/21] which IMO can be less performant. Though we know that Edge 14 has some issues with its fetch implementation [https://github.com/qubyte/fetch-ponyfill/issues/9#issuecomment-253911888]

So should we default to the global fetch or not?
Doing so would also simplify the mocking a lot, right? We could just set the mocked fetch as global and voila.

@pimterry discuss :)

Right now the refreshToken param defaults to true, and in that case the request to /whoami is sent. If the baseUrl is empty then it fails because fetch doesn't know what to do

I think the best thing to do is make the apiUrl an optional param to the request factory and use it for the /whoami request in the send method. If apiUrl is not passed and refreshToken is set err quickly and always (not only when the token is expired).

Additionally make the default refreshToken value an option to the factory.

Or at least specify in docs that the baseUrl is required and must be the API URL (which is kinda strange as other than that the module allows sending arbitrary requests to any hosts, but uses the baseUrl for the /whoami request.

Reported here balena-io/balena-sdk#257

@craig-mulligan what env does it happen in? Chrome? Node? Something else?

At least in the recent version of Chrome.
Or even worse it's a getter and not a normal member.

As a result overriding it here https://github.com/resin-io-modules/resin-request/blob/master/lib/request.coffee#L145 doesn't work and it's always the same original ReadableStream.

Interestingly it's different in the test env (PhantomJS) so the tests didn't catch it.

Namely when using the resin-sdk's logs.subscribe() method on Edge the network panel shows a 401 on an HTTP/2 request

Name	Protocol	Method	Result	Content type	Received	Time	Initiator
https://api.resinstaging.io/device/v2/2876517021a5ca34f977f084132540ae/logs?count=0&stream=1	HTTP/2	GET	401	text/plain	12 B	383.15 ms	Fetch

Accept: */*
Accept-Encoding: gzip, deflate
Host: api.resinstaging.io
map: [object Object]
Origin: https://dashboard.resinstaging.io
Referer: https://dashboard.resinstaging.io/devices/2876517021a5ca34f977f084132540ae/summary
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.79 Safari/537.36 Edge/14.14393

See: https://github.com/resin-io/resin-ui/issues/2280

See balena-io-modules/balena-register-device#29 (comment)

With empty directory in node it does not make a request to /whoami
But in the browser it tries to refresh the token even though the localStorage is supposed to be empty (it's a fresh PhantomJS instance).

Currently requests that timeout have their promises rejected, but the underlying request isn't actually aborted, the result is just ignored.

It's hard to do this on the frontend (see whatwg/fetch#20: it's not going to be specced for a while, let alone implemented), but node-fetch has support through the timeout option.

The module scans your environment for a saved session token. Alternatively, you may pass the apiKey options. Otherwise, the request is made anonymously.

Should be replaced

Node 18 now has native fetch and webstreams are available since node 16.
On the other hand we might want to consider moving to a different more feature rich library like got.
See: https://balena.zulipchat.com/#narrow/stream/346007-balena-io.2FbalenaCloud/topic/What.20we.20replace.20'request'.20with.3F