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

feat: Controller.fetch returns denoramlized form #2545

Merged
merged 1 commit into from Mar 30, 2023
Merged

Conversation

ntucker
Copy link
Collaborator

@ntucker ntucker commented Mar 30, 2023

Fixes #2330 .

Motivation

@taystack @gregor-mueller @tobobo

There is a strong desire to have Controller.fetch return the same value as hooks like useSuspense and useCache would. Furthermore, currently the return type is often 'any' because default RestEndpoint without a process method will have that resolve type.

Solution

The new version simply runs its through denormalize and we use the same typing value as useSuspense.

I realized, denormalize works fine without any entities, so there's no need for any special code - it will construct the expected return values even with 'flat' inputs. This eliminates the need for a more cpu intensive normalize/denormalize step.

This does mean since it has no data from the store, the updated values from something like useCache will likely not match. However, I believe this more closely matches a user's expectation - they simply want that fetches value, but in the same form they expect with responses. This allows utilizing getters, and other fun class values.

Usage

We use the new standard for breaking changes shipping with /next exports, with updates to the normal exports in the next breaking version.

Note import from @rest-hooks/react/next

import { useController } from '@rest-hooks/react/next';
import { TodoResource, Todo } from './api/Todo';

export function TodoItem({ todo }: { todo: Todo }) {
  const ctrl = useController();
  const handleChange = async e => {
      const todo = await ctrl.fetch(
        TodoResource.partialUpdate,
        { id: todo.id },
        { completed: e.currentTarget.checked },
      );
      // todo is Todo, we can use all its members and be type-safe
      console.log(todo.pk(), todo.title);
  };
  return (
    <label style={{ display: 'block' }}>
      <input
        type="checkbox"
        checked={todo.completed}
        onChange={handleChange}
      />
      {todo.completed ? <strike>{todo.title}</strike> : todo.title}
    </label>
  );
}

@changeset-bot
Copy link

changeset-bot bot commented Mar 30, 2023

🦋 Changeset detected

Latest commit: f79601d

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

This PR includes changesets to release 5 packages
Name Type
@rest-hooks/react Minor
@rest-hooks/rest Patch
@rest-hooks/core Minor
@rest-hooks/legacy Patch
example-benchmark 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

@vercel
Copy link

vercel bot commented Mar 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
rest-hooks ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 30, 2023 at 8:18PM (UTC)

@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Patch coverage: 95.37% and no project coverage change.

Comparison is base (0a6c93c) 98.11% compared to head (075ebd9) 98.11%.

❗ Current head 075ebd9 differs from pull request most recent head f79601d. Consider uploading reports for the commit f79601d to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2545   +/-   ##
=======================================
  Coverage   98.11%   98.11%           
=======================================
  Files         124      125    +1     
  Lines        2067     2071    +4     
  Branches      306      306           
=======================================
+ Hits         2028     2032    +4     
  Misses         18       18           
  Partials       21       21           
Impacted Files Coverage Δ
packages/core/src/controller/BaseController.ts 95.23% <95.23%> (ø)
packages/core/src/controller/Controller.ts 100.00% <100.00%> (+4.54%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ntucker ntucker merged commit 775352c into master Mar 30, 2023
21 checks passed
@ntucker ntucker deleted the controller-fetch branch March 30, 2023 20:13
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.

Missing type inference for controller.fetch
1 participant