Skip to content

refactor(create-react-router): use native Fetch #14140

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

MichaelDeBoey
Copy link
Member

As mentioned in remix-run/web-std-io#42 (comment), we probably don't need to use the agent option, so we can just use native Fetch API completely


I'm sure people from the wider @e18e ecosystem cleanup (like @43081j, @benmccann, @Fuzzyma, @outslept & @talentlessguy) will be very happy to see these kind of changes as well

@MichaelDeBoey MichaelDeBoey added dependencies Pull requests that update a dependency file pkg:create-react-router labels Aug 7, 2025
Copy link

changeset-bot bot commented Aug 7, 2025

⚠️ No Changeset found

Latest commit: ccbe293

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@MichaelDeBoey MichaelDeBoey force-pushed the use-native-fetch-in-crr branch from 6a09814 to ccbe293 Compare August 7, 2025 18:42
@MichaelDeBoey
Copy link
Member Author

Tests are failing because we don't support env var proxy usage anymore with this change.

Not sure if this is a use-case we want to support tbh 🤔
Imo, this is a use-case that wouldn't really occur IRL

CC/ @markdalgleish is there any specific use-case you had in mind when implementing this?

@timdorr
Copy link
Member

timdorr commented Aug 8, 2025

FYI, this was added way back in remix-run/remix#4159

@timdorr
Copy link
Member

timdorr commented Aug 8, 2025

And note this may be less of an issue now that Node natively supports proxies: https://nodejs.org/en/blog/release/v24.5.0#built-in-proxy-support-in-request-and-agent

@MichaelDeBoey
Copy link
Member Author

@timdorr We're still supporting Node v20 (even Node v18 for create-react-router), so we can't rely on native proxy support

@timdorr
Copy link
Member

timdorr commented Aug 8, 2025

I know, I'm just saying there's a "fix" for users that might need support for proxies.

Alternatively, we could import a version of unidici that supports proxies as well.

@brophdawg11
Copy link
Contributor

Can we just do nothing? create-react-router is working fine today on the current version of @remix-run/web-fetch. This all stemmed from chatter on remix-run/web-std-io#42, which we can just close because we don't need it. We can mark the repo as deprecated/maintenance mode, etc. But I don't think that we need to do anything to RRv7 here.

@timdorr
Copy link
Member

timdorr commented Aug 8, 2025

Since create-react-router is normally run through npx and installed for each usage, the install size actually does make a notable impact on performance. @remix-run/web-fetch and proxy-agent bring 73 dependencies and about 29MB of data into that installation process, so not anything insignificant.

It comes at a cost of not supporting proxy users running node < 24.5.0, so that's really the decision to be made here. Personally, I think it's worth it.

@MichaelDeBoey
Copy link
Member Author

I'm with @timdorr here and I think the benefits are greater than the drawbacks imo.

@talentlessguy
Copy link

Couldn't it be conditionally installed and used depending on node version? or it's something not really possible with node

@timdorr
Copy link
Member

timdorr commented Aug 8, 2025

No, this is part of the package.json, so there's no ability to make something optionally installed in that context.

@brophdawg11
Copy link
Contributor

yeah I don't know what the usage is for proxy users, but it sounds like it would be a breaking change so it's probably a non-starter. No one has ever complained about the execution time of npx create-react-router AFAIK so this feels fine to wait for v8 to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed dependencies Pull requests that update a dependency file pkg:create-react-router
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants