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

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

Closed
1 task done
lensbart opened this issue Jul 30, 2023 · 16 comments
Closed
1 task done
Assignees

Comments

@lensbart
Copy link
Contributor

lensbart commented Jul 30, 2023

What version of Remix are you using?

1.19.1

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

  • Yes

Steps to Reproduce

  1. yarn create remix with default settings (my-remix-app, Just the basics, Remix App Server, TypeScript, run yarn install)
  2. cd my-remix-app
  3. 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>
  4. yarn set version berry
  5. yarn (you should see the message “automatically enabling the compatibility node-modules linker 👍”, a line nodeLinker: node-modules will be added to .yarnrc.yml)
  6. When using VSCode: open command panel (⇧ ⌘ P on macOS) → TypeScript: Select TypeScript Version…Use Workspace Version
  7. Observe that the inferred type of data is still (correctly) SerializeObject<UndefinedToOptional<{ some: string; }>>
  8. 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
  9. Run yarn dlx @yarnpkg/sdks vscode
  10. You’ll have to set the VSCode TypeScript version again like in step 6
  11. 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 lensbart changed the title useLoaderData<typeof loader>() doesn’t work well with Yarn PnP Type of useLoaderData<typeof loader>() doesn’t get inferred when using Yarn PnP Jul 30, 2023
@lensbart
Copy link
Contributor Author

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
Copy link
Member

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

@lensbart
Copy link
Contributor Author

lensbart commented Jul 31, 2023

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
Copy link
Member

@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: #7137

@lensbart
Copy link
Contributor Author

@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
Copy link
Member

Closed by #7137

@github-actions
Copy link
Contributor

🤖 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
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@markdalgleish
Copy link
Member

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 added a commit to nextbook/serialize-from that referenced this issue Aug 21, 2023
@lensbart
Copy link
Contributor Author

@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.

Screenshot 2023-08-22 at 00 02 03

Cmd+click on useLoaderData:

Screenshot 2023-08-22 at 00 02 23

@markdalgleish
Copy link
Member

@lensbart Sorry, I meant with the nightly version.

@lensbart
Copy link
Contributor Author

@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.

Screenshot 2023-08-22 at 00 15 09

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

@lensbart
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Merged
Development

No branches or pull requests

4 participants