Type of `useLoaderData<typeof loader>()` doesn’t get inferred when using Yarn PnP

lensbart commented 1 year ago

What version of Remix are you using?

1.19.1

Are all your remix dependencies & dev-dependencies using the same version?

[X] Yes

Steps to Reproduce

yarn create remix with default settings (my-remix-app, Just the basics, Remix App Server, TypeScript, run yarn install)
cd my-remix-app

Make the following changes:

diff --git a/app/root.tsx b/app/root.tsx
index 8cb74a167..ad3a65b41 100644
--- a/app/root.tsx
+++ b/app/root.tsx
@@ -7,13 +7,17 @@
import {
  Outlet,
  Scripts,
  ScrollRestoration,
+  useLoaderData,
} from "@remix-run/react";

export const links: LinksFunction = () => [
  ...(cssBundleHref ? [{ rel: "stylesheet", href: cssBundleHref }] : []),
];

+export const loader = () => ({ some: "data" });
+
export default function App() {
+  const data = useLoaderData<typeof loader>();
  return (
    <html lang="en">
      <head>

yarn set version berry
yarn (you should see the message “automatically enabling the compatibility node-modules linker 👍”, a line nodeLinker: node-modules will be added to .yarnrc.yml)
When using VSCode: open command panel (⇧ ⌘ P on macOS) → TypeScript: Select TypeScript Version… → Use Workspace Version
Observe that the inferred type of data is still (correctly) SerializeObject<UndefinedToOptional<{ some: string; }>>
Remove nodeLinker: node-modules from .yarnrc.yml (or change it to nodeLinker: pnp) and run yarn again. This will remove your node_modules folder and enable the Plug‘n’Play module resolution mechanism
Run yarn dlx @yarnpkg/sdks vscode
You’ll have to set the VSCode TypeScript version again like in step 6

Observe that the inferred type of data is now SerializeFrom<T>. Cmd+clicking on useLoaderData will go to line 114 in components.d.ts:

/**
 * Returns the JSON parsed data from the current route's `loader`.
 *
 * @see https://remix.run/hooks/use-loader-data
 */
export declare function useLoaderData<T = AppData>(): SerializeFrom<T>;

but it’s not possible to Cmd+click further on SerializeFrom.

Expected Behavior

The inferred type of useLoaderData<typeof loader>() remains SerializeObject<UndefinedToOptional<{ some: string; }>> as in step 7

Actual Behavior

The inferred type of useLoaderData<typeof loader>() is SerializeFrom<T> as described in step 11

lensbart commented 1 year ago

I have a hunch that the type of SerializeFrom can’t be inferred because the package.json of @remix-run/react has "typings": "dist/index.d.ts", and that file doesn’t export SerializeFrom

MichaelDeBoey commented 1 year ago

Did you try wrapping your response in json helper function? It's not recommended to return pure JS objects

lensbart commented 1 year ago

Did you try wrapping your response in json helper function?

The problem is with the underlying type of SerializeFrom not being accessible. The type of loader is correctly inferred in both cases:

() => { some: string; } following the steps outlined above
() => TypedResponse<{ some: string; }> when the return value is wrapped in json

markdalgleish commented 1 year ago

@lensbart Thanks for the tip about the underlying SerializeFrom type. Looks like this is due to the fact that @remix-run/server-runtime is a dev dependency. I've just opened a PR fixing this: https://github.com/remix-run/remix/pull/7137

lensbart commented 1 year ago

@markdalgleish good to hear!

Semi-relatedly, I think it would be a good idea to upgrade the repository version of Yarn so that it imposes stricter rules on dependencies. (There have been several issues with Yarn that would have been caught by using Yarn 2 or higher.)

MichaelDeBoey commented 1 year ago

Closed by #7137

github-actions[bot] commented 1 year ago

🤖 Hello there,

We just published version v0.0.0-nightly-65fdbcd-20230815 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

lensbart commented 1 year ago

@markdalgleish I installed the nightly version (for all my remix dependencies), but this doesn’t seem to work. The behaviour is the same as before. Did you get a different result?

Thanks.

markdalgleish commented 1 year ago

@lensbart The nightly version has fixed the initial issue where SerializeFrom wasn't available, but I'm now getting a different issue where the type of data is Record<string, number>. SerializeFrom internally uses Jsonify from type-fest, and interestingly enough I was able to reproduce this issue even when using Jsonify directly in a Yarn PnP project. I was able to reproduce this in the TS playground too:

https://www.typescriptlang.org/play?ts=5.1.6#code/JYWwDg9gTgLgBDAnmApnA3nAyiqwCGANsAF4oBiUEIcAvnAGZU0BEAAlCiMAB4C0UAK4A7APQBnXADdcAkTFAoWAbgBQoSLATI0mAFLiIw4A0R1GzOCySo+DFOJgrVqlD03wbaAEpdeAGQh8ABNcABF8GHw4AF5sXAJiMkpqAB4ACgBKWIA+DDhDEBQALjgAcmDI-DK6HLVRUTgm5paAPQB+VQaWnpbAXg3AWR2XV3doTx04AyMTRECQ8KrYycNjU1TMQpLyyqia2jquxt7mjsPj3sA+DcB2XcBYAkAhMkB4P9UgA

Looks like this might be surfacing an upstream issue? Or am I missing something?

lensbart commented 1 year ago

At this point I find it difficult to help, because it doesn’t match the issue I’m having (TypeScript being seemingly unable to “resolve” the underlying type of SerializeFrom). Did you try exporting SerializeFrom from the remix package that uses it?

lensbart commented 1 year ago

I would also assume that the issue on the TypeScript playground is unrelated to Yarn PnP?

markdalgleish commented 1 year ago

Could you share a minimal repo to make sure we're looking at the same thing?

I realise the TS playground is unrelated to Yarn PnP, but I was using it to try and see what the expected behaviour should be in a clean environment and was surprised to see a similar issue.

lensbart commented 1 year ago

@markdalgleish Sure! Here you go: https://github.com/nextbook/serialize-from I just followed the steps in the first message in this issue (should it be reopened?). Let me know if I can provide any further information.

Cmd+click on useLoaderData:

markdalgleish commented 1 year ago

@lensbart Sorry, I meant with the nightly version.

lensbart commented 1 year ago

@markdalgleish my bad. When I upgrade the remix dependencies to 0.0.0-nightly-65fdbcd-20230815, I see the exact same thing as you described.

I’ll try to come up with a minimal reproduction for the issue I had earlier, after upgrading.

lensbart commented 1 year ago

@markdalgleish Maybe I was confused and did something wrong when posting this message. Using the nightly seems to resolve my initial issue now. Apologies for any time lost.

I don’t have the issue with Record<string, number> in my main repository, but not sure if that’s relevant since we have a reproduction already.

remix-run / remix