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.

Sign up for GitHub

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

Jump to bottom

fetch: rewrite with composability in mind #131

Merged
merged 18 commits into from
Jul 30, 2022
Merged

fetch: rewrite with composability in mind #131

merged 18 commits into from
Jul 30, 2022

Conversation

atk
Copy link
Member

@atk atk commented May 17, 2022

Fresh from the discussion here #129, this is the first draft of a simplified, more composable fetch primitive.

This is still WIP; the retry and cache modifier are not yet done, the internal documentation and the README are not yet finished.

@atk atk marked this pull request as draft May 18, 2022 20:45
@davedbase
Copy link
Member

Re: Axios I have a lot of feelings about supporting it. This 3 year old post comes to mind: https://www.reddit.com/r/reactjs/comments/dcte0e/psa_axios_is_mostly_dead/. If you are going to support it perhaps consider using redaxios instead? It's an 800 byte alternative written by Jason Miller: https://github.com/developit/redaxios

@atk
Copy link
Member Author

atk commented May 21, 2022

I'll support anything with the same API.

@atk
Copy link
Member Author

atk commented May 21, 2022

Also, I plan on supporting TRPC in the near future.

@davedbase
Copy link
Member

I'm wondering if using Redaxios out of the box as a dependency is better than Axios. The user may not care which is used so opting for the smaller is an advantage (6.7kb vs 1kb). The user can install Axios proper if they need to override it for whatever reason. Just a thought/suggestion :-)

@thetarnav
Copy link
Member

@davedbase Wouldn't all of them be optional dependencies that user has to explicitly install if he want to use it?

@davedbase
Copy link
Member

Yah you're right, sorry I thought axios was marked as a dependency in the package.json but I read it wrong. Maybe some instructions/suggestions in the readme for the axios modifier would be best?

@atk
Copy link
Member Author

atk commented May 22, 2022

We should definitely document this thoroughly.

@atk
Copy link
Member Author

atk commented May 24, 2022

I've added the first stub for caching. The reload mechanism is still missing. I've put cache into its own file so modifiers is not blown up out of proportion.

@atk
Copy link
Member Author

atk commented Jun 5, 2022

refetch mechanism is also added now. I'm closer to finishing the main functionality and can then turn to types and tests.

@atk atk force-pushed the fetch-overhaul branch 2 times, most recently from f740b7c to 6ce1549 Compare July 18, 2022 11:44
@thetarnav thetarnav mentioned this pull request Jul 24, 2022
Merged
@atk atk force-pushed the fetch-overhaul branch 2 times, most recently from 3fbb1c7 to 4cb9caf Compare July 27, 2022 09:03
@atk atk changed the title [WIP]: fetch: rewrite with composability in mind fetch: rewrite with composability in mind Jul 27, 2022
thetarnav
thetarnav reviewed Jul 28, 2022
View reviewed changes
Copy link
Member

@thetarnav thetarnav left a comment

Choose a reason for hiding this comment

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

Here are a couple of things I've caught. Generally, the amount of work put into this package is insane. Really good stuff.

packages/fetch/src/fetch.ts Outdated Show resolved Hide resolved
packages/fetch/src/modifiers.ts Outdated Show resolved Hide resolved
packages/fetch/src/modifiers.ts Outdated Show resolved Hide resolved
packages/fetch/src/fetch.ts Show resolved Hide resolved
packages/fetch/src/server.ts Outdated Show resolved Hide resolved
packages/fetch/src/modifiers.ts Show resolved Hide resolved
packages/fetch/src/cache.ts Show resolved Hide resolved
@atk atk marked this pull request as ready for review July 29, 2022 12:53
@atk atk requested a review from thetarnav July 29, 2022 12:53
@changeset-bot
Copy link

changeset-bot bot commented Jul 30, 2022

🦋 Changeset detected

Latest commit: 372f964

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

This PR includes changesets to release 1 package
Name Type
@solid-primitives/fetch Major

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

thetarnav
thetarnav approved these changes Jul 30, 2022
View reviewed changes
Copy link
Member

@thetarnav thetarnav left a comment

Choose a reason for hiding this comment

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

Looks good, let's ship it :)

@thetarnav thetarnav merged commit 14e6156 into main Jul 30, 2022
@thetarnav thetarnav deleted the fetch-overhaul branch July 30, 2022 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thetarnav thetarnav thetarnav approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@atk @davedbase @thetarnav