Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

myarmolinsky
Copy link
Contributor

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

Change-type: patch

@myarmolinsky myarmolinsky self-assigned this Jul 5, 2024
@dfunckt
Copy link
Collaborator

dfunckt commented Jul 5, 2024

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
Copy link
Contributor

thgreasi commented Jul 5, 2024

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
Copy link
Collaborator

dfunckt commented Jul 5, 2024

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
Copy link
Contributor

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).

@thgreasi thgreasi closed this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants