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

Fix pipeline types #410

Merged
merged 3 commits into from
Apr 16, 2024
Merged

Fix pipeline types #410

merged 3 commits into from
Apr 16, 2024

Conversation

juliangruber
Copy link
Member

@juliangruber juliangruber commented Apr 16, 2024

Tests are failing locally, but not in CI. This is because we didn't include the build step in CI. This PR fixes that. It also then disables it temporarily, so that we can publish new versions of this project again - tsc isn't passing.

@bajtos I wasn't able to figure out the type failures unfortunately :| Do you have an idea?

@juliangruber juliangruber merged commit 87acd46 into main Apr 16, 2024
18 checks passed
@juliangruber juliangruber deleted the fix/pipeline-types branch April 16, 2024 12:11
@juliangruber
Copy link
Member Author

Hindsight review please, I need to get an urgent release out

@bajtos
Copy link
Member

bajtos commented Apr 18, 2024

@bajtos I wasn't able to figure out the type failures unfortunately :| Do you have an idea?

When I oversimplify the problem:

  • pipeline expects the first argument to be Node.js ReadableStream and the second argument to be Node.js WritableStream
  • res.body has an Unidici-specific readable-stream type that's similar to Node.js readable-stream type but not fully compatible from TypeScript's point of view
  • tar.x returns tar-specific type Unpack, which is similar to Node.js writable-stream but not fully compatible from TypeScript's point of view

This could be also a problem of @types/node or the typings provided by Undici or Tar.

I'll open a PR to fix this.

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.

2 participants