search

balena-io-modules / balena-compose

Complete toolkit for building docker-compose.yml files and optionally deploy them on balenaCloud
Apache License 2.0
8 stars 0 forks source link

Replace pinejs-client-request with pinejs-client-fetch #51

Closed myarmolinsky closed 1 month ago

myarmolinsky commented 2 months ago

Replace pinejs-client-request with pinejs-client-fetch

Change-type: patch

dfunckt commented 2 months ago

Looking at pine-client-fetch source, it doesn't seem that it understands retrying params: https://github.com/balena-io-modules/pinejs-client-fetch/blob/master/src/index.ts which makes it functionally inferior.

thgreasi commented 2 months ago

May I ask what's the motivation behind this change?

Retries are actually implemented in pinejs-client-core, so maybe pinejs-client-fetch just needs a pinejs-client-core bump.

On the other hand pinejs-client-fetch doesn't use etags and does not respecting the retry-after header. The latter is big blocker for my, and I would be be strongly against such a change/regression, since that would break both the cli & the builder experience for releases with many components (services/env vars/etc). See: https://github.com/balena-io-modules/pinejs-client-request/blob/2f28a3c879727a80efac698ea11dc22b887dec6b/request.ts#L71-L83

dfunckt commented 2 months ago

May I ask what's the motivation behind this change?

Is the fact that request.js has been abandoned several years ago motivation enough?

Retries are actually implemented in pinejs-client-core, so maybe pinejs-client-fetch just needs a pinejs-client-core bump.

Good to know. I remember pine-client-request doing something related to retrying but turns out it's actually this:

On the other hand pinejs-client-fetch doesn't use etags and does not respecting the retry-after header. The latter is big blocker for my, and I would be be strongly against such a change/regression, since that would break both the cli & the builder experience for releases with many components (services/env vars/etc). See: https://github.com/balena-io-modules/pinejs-client-request/blob/2f28a3c879727a80efac698ea11dc22b887dec6b/request.ts#L71-L83

Thanks for clarifying.

thgreasi commented 1 month ago

Replaced by #52 which dropped the createClient method on favor of requiring consumers to be passing their own preferred pinejs-client instance (which was already supported).