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

refactor: stop using node-fetch inside create-remix #7345

Merged
merged 6 commits into from
Dec 8, 2023

Conversation

MichaelDeBoey
Copy link
Member

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Sep 5, 2023

🦋 Changeset detected

Latest commit: 1400b0d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/dev Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@MichaelDeBoey MichaelDeBoey changed the title chore: stop using node-fetch refactor: stop using node-fetch Sep 5, 2023
@MichaelDeBoey MichaelDeBoey force-pushed the stop-using-node-fetch branch 4 times, most recently from a2d9002 to 8d34c47 Compare September 5, 2023 18:37
@brophdawg11
Copy link
Contributor

This doesn't need to go into 2.0.0 right? With the stable right around the corner we want to put as little into release-next that isn't a critical bug fix or breaking change. Can we do this against dev for a subsequent 2.x release? There's some sort of typing issue with .pipe as well it looks like.

@MichaelDeBoey MichaelDeBoey changed the base branch from release-next to dev September 6, 2023 19:41
@brophdawg11
Copy link
Contributor

I'm not sure why CI isn't running here but this has a TS error for me locally:

$ /Users/matt/me/shopify/repos/remix/node_modules/.bin/tsc -b
packages/create-remix/copy-template.ts(309,21): error TS2339: Property 'pipe' does not exist on type 'ReadableStream<Uint8Array>'.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Error: yarn exited with code 2
    at ChildProcess.<anonymous> (file:///Users/matt/me/shopify/repos/remix/scripts/build.mjs:36:16)
    at ChildProcess.emit (node:events:514:28)
    at maybeClose (node:internal/child_process:1105:16)
    at ChildProcess._handle.onexit (node:internal/child_process:305:5)
error Command failed with exit code 1.

Looks like it's from our gzip logic, so we'll need to figure out how to do that with web streams:

await pipeline(
  response.body.pipe(gunzip()),
  tar.extract(downloadPath, { ... })
)

@brophdawg11 brophdawg11 changed the title refactor: stop using node-fetch refactor: stop using node-fetch inside create-remix Dec 8, 2023
@brophdawg11
Copy link
Contributor

Confirmed with @jacob-ebey as well that the changes to the tar gzip extraction looks right

@brophdawg11 brophdawg11 merged commit c0d1b20 into remix-run:dev Dec 8, 2023
9 checks passed
@MichaelDeBoey MichaelDeBoey deleted the stop-using-node-fetch branch December 12, 2023 01:12
Copy link
Contributor

🤖 Hello there,

We just published version 2.4.0-pre.9 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 2.4.1-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 2.4.1 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants