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

use undici in place of node-fetch #17632

Closed
wants to merge 2 commits into from

Conversation

punkle
Copy link
Contributor

@punkle punkle commented May 3, 2023

Hey, I just made a Pull Request!

use undici in place of node-fetch

The purpose of this patch is to attempt to fix an error that is appearing in the scaffolder fetch:plain:file action. It is throwing this error:

InternalServerError: stream is not readable

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

The purpose of this patch is to attempt to fix an error that is
appearing in the scaffolder fetch:plain:file action. It is throwing
this error:

`InternalServerError: stream is not readable`

Signed-off-by: Brian Fletcher <brian@roadie.io>
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented May 3, 2023

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/backend-common packages/backend-common patch v0.18.5-next.1

Signed-off-by: Brian Fletcher <brian@roadie.io>
@@ -22,7 +22,7 @@ import {
ScmIntegrations,
} from '@backstage/integration';
import { RestEndpointMethodTypes } from '@octokit/rest';
import fetch, { RequestInit, Response } from 'node-fetch';
import { fetch, Response, RequestInit } from 'undici';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clearly its just not this simple.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@punkle nope 😂 I would start from a fork of this repo and get things setup so that you can reproduce the error locally with the current setup, and then replace it in the GithubUrlReader and see what happens on your machine to test it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we would want to just ship it for the GIthubUrlReader you see either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah. I probably should have made this a draft. Ive no intension of proposing this be merged without knowing it works. I suppose my patch demostrates though that ReadUrlResponseFactory#fromNodeJSReadable and ReadUrlResponseFactory#fromReadable cannot be used in the same way with undici.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha nice ok just wanted to make sure we're on the same level 😅 , and I think it's because we would need to use ]
import { stream } from 'undici' as the request method not fetch. The whatwg-fetch spec does not have any specification on response.body being a stream so theres a separate API for it as it tries to keep with the web spec.

@punkle punkle marked this pull request as draft May 3, 2023 15:10
@gavlyukovskiy
Copy link
Contributor

Thank you @punkle, I've been able to reproduce this issue on existing implementation of GithubUrlReader (not consistently but more like 3 out of 5 times) and I also suspect that something was wrong with underlying node-fetch library as there were no obvious triggers, but sometimes stream has been closed before we could read it.
I will try to test this PR in a similar environment when I have a little bit more time this/next week.

@punkle
Copy link
Contributor Author

punkle commented May 5, 2023

@gavlyukovskiy this patch is not working at this point unfortunately. Ill try get back to it at some point.

@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label May 12, 2023
@github-actions github-actions bot closed this May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants