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

useActionData returning incorrect data #10074

Open
ramiroazar opened this issue Oct 8, 2024 · 3 comments
Open

useActionData returning incorrect data #10074

ramiroazar opened this issue Oct 8, 2024 · 3 comments

Comments

@ramiroazar
Copy link
Contributor

ramiroazar commented Oct 8, 2024

Reproduction

  1. Open reproduction https://stackblitz.com/edit/remix-run-remix-vrher6
  2. Navigate to /test route
  3. Add a value to the form text input
  4. Click submit button
  5. View data logged in browser console

I originally encountered this using the Cloudflare template with @remix-run/cloudflare, but I replicated this in the reproduction using @remix-run/node.

System Info

System:
    OS: macOS 12.6.7
    CPU: (4) x64 Intel(R) Core(TM) i5-5350U CPU @ 1.80GHz
    Memory: 20.57 MB / 8.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 20.14.0 - ~/.nvm/versions/node/v20.14.0/bin/node
    Yarn: 1.22.4 - /usr/local/bin/yarn
    npm: 10.7.0 - ~/.nvm/versions/node/v20.14.0/bin/npm
  Browsers:
    Brave Browser: 124.1.65.132
    Chrome: 129.0.6668.90
    Safari: 15.6.1
  npmPackages:
    @remix-run/cloudflare: 2.12.1 => 2.12.1 
    @remix-run/cloudflare-pages: 2.12.1 => 2.12.1 
    @remix-run/dev: 2.12.1 => 2.12.1 
    @remix-run/react: 2.12.1 => 2.12.1 
    @remix-run/testing: 2.12.1 => 2.12.1 
    vite: ^5.1.0 => 5.2.12

Used Package Manager

npm

Expected Behavior

The useActionData hook should just return the serialised data from the most recent route action.

Actual Behavior

The useActionData hook does return the correct data, but within another data property as part of an object with other properties such as abortPromise, deferredKeys, pendingKeysSet, etc.

This doesn't match the types from useActionData<typeof action>(), so trying to access the data property throws the following type error.

Property 'data' does not exist on type '({} & { text: string | JsonifyObject<File> | null; } & {}) | undefined'.

The only related issue I could find is remix-run/blues-stack#151.

@laishere
Copy link

laishere commented Oct 8, 2024

I think the problem is action does not support defer.

Check react-router sources here:

loader handles DeferedResult:
https://github.com/remix-run/react-router/blob/58d8f316e0c682efaaa012125ff152ffa9b36ec8/packages/router/router.ts#L5267

action throws an error when it is `DeferedResult``:
https://github.com/remix-run/react-router/blob/58d8f316e0c682efaaa012125ff152ffa9b36ec8/packages/router/router.ts#L1796-L1798

However, handleAction does not throw an error as expected.

After debugging, I found the problem here:
https://github.com/remix-run/react-router/blob/58d8f316e0c682efaaa012125ff152ffa9b36ec8/packages/router/router.ts#L4905

let handlerPromise: Promise<DataStrategyResult> = (async () => {
  try {
    let val = await (handlerOverride
      ? handlerOverride((ctx: unknown) => actualHandler(ctx))
      : actualHandler());
    return { type: "data", result: val };
  } catch (e) {
    return { type: "error", result: e };
  }
})();

return Promise.race([handlerPromise, abortPromise]);

The val that the above logic return is a Response, which will be treated as normal data result in convertDataStrategyResultToDataResult:
https://github.com/remix-run/react-router/blob/58d8f316e0c682efaaa012125ff152ffa9b36ec8/packages/router/router.ts#L5023-L5028

On the contrary, loader return a DefferedResult at the same place, which is the key difference:

// val of loader is a DefferedResult other than a Response
let val = await (handlerOverride
      ? handlerOverride((ctx: unknown) => actualHandler(ctx))
      : actualHandler()); 

The difference is caused by actualHandler.

For loader it is:

dataRoute.loader = async (

For action it is:

dataRoute.action = (

In conclusion, the action does not support defer like the loader. And dataRoute.action implemented in remix always return a response, which makes the expected DeferredResult error will never be thrown in react-route.

Now that react-router doesn't like defer in action, I think we should update the docs of defer and throw an error in remix when people try to do so.

@brophdawg11
Copy link
Contributor

@laishere is correct in that actions do not support defer (and never have). It looks like somewhere along the way we lost the error message that used to be thrown when you tried that.

It's worth noting that with Single Fetch this limitation is removed and you can return promises from actions in plain javascript objects, no need for defer.

@laishere
Copy link

@brophdawg11 I believe it's because the action handler return a Response other than actual data, so it will always be treated as DataResult not DeferredResult in react-router here, which will bypass DeferredResult type check and no error will be thrown.

Action handler defined in remix:

dataRoute.action = (

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

No branches or pull requests

3 participants