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

enhance: createResource() -> resource() #3158

Merged
merged 1 commit into from
Jul 21, 2024
Merged

enhance: createResource() -> resource() #3158

merged 1 commit into from
Jul 21, 2024

Conversation

ntucker
Copy link
Collaborator

@ntucker ntucker commented Jul 20, 2024

Motivation

Reduced verbosity.

Solution

  • createResource() -> resource()
import { resource } from '@data-client/rest';

export const TodoResource = resource({
  urlPrefix: 'https://jsonplaceholder.typicode.com',
  path: '/todos/:id',
  searchParams: {} as { userId?: string | number } | undefined,
  schema: Todo,
  optimistic: true,
});

Open questions

Why not new Resource()?

Class works for RestEndpoint, because the 'project-wide' code sharing are all non-type specific - things that do not affect the type of RestEndpoint. RestEndpoint's typing comes from it's schema, and function signature which are expected to be different in each instance, with some small similarities being doable with .extend().

However, for resources, while there are a few non-typed overrides like optimisticPartial, getName, Endpoint, Collection, many cases we want to establish common method shapes. This means affecting the type of the resource. (Like paginationField default, or getList shape to handle pagination). Because of this, classes fundamentally do not work from a TypeScript perspective. TypeScript simply does not allow changes in types of descendants in any way but specialization (aka extends). This means it is impractical for many common cases of 'Base class' to use class inheritance, which means we would need to fallback to a function anyway. Having two dominant ways of doing things seems confusing, so we will go with 'lowest common denominator'.

Is resource() lower case with no action confusing?

Convention is to have instances use PascalCase, so having a variable named resource would be against convention. Since resource is camelCase, it is clear it should not be called with new

Copy link

changeset-bot bot commented Jul 20, 2024

🦋 Changeset detected

Latest commit: ca7787b

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

This PR includes changesets to release 1 package
Name Type
@data-client/rest 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

Copy link

codecov bot commented Jul 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.58%. Comparing base (8003ebb) to head (ca7787b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3158   +/-   ##
=======================================
  Coverage   98.58%   98.58%           
=======================================
  Files         131      131           
  Lines        2259     2260    +1     
  Branches      459      459           
=======================================
+ Hits         2227     2228    +1     
  Misses         21       21           
  Partials       11       11           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ntucker ntucker requested a review from notwillk July 21, 2024 10:15
@ntucker ntucker enabled auto-merge July 21, 2024 16:33
@ntucker ntucker disabled auto-merge July 21, 2024 16:33
@ntucker ntucker merged commit 34e2e51 into master Jul 21, 2024
25 checks passed
@ntucker ntucker deleted the resource-name branch July 21, 2024 16:33
@github-actions github-actions bot mentioned this pull request Jul 21, 2024
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.

1 participant