remix-run / remix

Build Better Websites. Create modern, resilient user experiences with web fundamentals.
https://remix.run
MIT License
29.74k stars 2.51k forks source link

`useActionData` returning incorrect data #10074

Open ramiroazar opened 1 week ago

ramiroazar commented 1 week ago

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 https://github.com/remix-run/blues-stack/issues/151.

laishere commented 1 week ago

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: https://github.com/remix-run/remix/blob/5954ad1520e921483fbe07f5995c6f9d92e7a37b/packages/remix-react/routes.tsx#L333

For action it is: https://github.com/remix-run/remix/blob/5954ad1520e921483fbe07f5995c6f9d92e7a37b/packages/remix-react/routes.tsx#L383

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 commented 1 week ago

@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 commented 1 week ago

@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: https://github.com/remix-run/remix/blob/5954ad1520e921483fbe07f5995c6f9d92e7a37b/packages/remix-react/routes.tsx#L383