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

Wrong typings generated when importing types from a package in a Rush monorepo (PNPM hard links) #967

Open
RIP21 opened this issue Feb 1, 2021 · 12 comments · Fixed by ezolenko/rollup-plugin-typescript2#332
Labels
kind: bug Something isn't working scope: upstream Issue in upstream dependency solution: duplicate This issue or pull request already exists solution: workaround available There is a workaround available for this issue topic: rollup-plugin-typescript2 Issues and PRs relating to rpts2

Comments

@RIP21
Copy link

RIP21 commented Feb 1, 2021

Current Behavior

I run tsdx build --transpileOnly

From this source

import { useToast as useChakraToast } from '@chakra-ui/react'
import { useMemo } from 'react'
import type { UseToastOptions } from '@chakra-ui/react'

export const useToast = (options?: UseToastOptions) => {
  const defaultOptions = useMemo(() => {
    return {
      position: 'top-right' as UseToastOptions['position'],
      status: 'success' as UseToastOptions['status'],
      isClosable: true,
      ...(options ?? {}),
    }
  }, [options])
  return useChakraToast(defaultOptions)
}

It generates this d.ts:

export declare const useToast: (options?: any) => any;

Expected behavior

Should output (and this is what regular tsc with emitDeclarationsOnly does):

import type { UseToastOptions } from '@chakra-ui/react';
export declare const useToast: (options?: UseToastOptions | undefined) => {
    (options?: UseToastOptions | undefined): string | number | undefined;
    close: (id: string | number) => void;
    closeAll: (options?: import("@chakra-ui/react").CloseAllToastsOptions | undefined) => void;
    update(id: string | number, options: Pick<UseToastOptions, "status" | "render" | "position" | "duration" | "onCloseComplete" | "title" | "description" | "isClosable" | "variant">): void;
    isActive: (id: string | number) => boolean | undefined;
};

Additional context

It's Rush.js managed monorepo so it uses PNPM so hard links to link modules into node_modules.
I'm using TS 4. A similar workaround mentioned here #926 (using PNPM fashioned resolutions) is applied.

If built without --transpileOnly fails with

app-frontend/libraries/hooks/common/src/useToast.ts(1,10): semantic error TS2305: Module '"../node_modules/@chakra-ui/react/dist/types"' has no exported member 'useToast'.

Your environment

  System:
    OS: Linux 5.3 Ubuntu 20.04.1 LTS (Focal Fossa)
    CPU: (16) x64 AMD Ryzen 7 3700X 8-Core Processor
    Memory: 449.17 MB / 31.37 GB
    Container: Yes
    Shell: 5.8 - /usr/bin/zsh
  Binaries:
    Node: 14.15.4 - ~/.nvm/versions/node/v14.15.4/bin/node
    Yarn: 1.22.10 - ~/.nvm/versions/node/v14.15.4/bin/yarn
    npm: 6.14.10 - ~/.nvm/versions/node/v14.15.4/bin/npm
  Browsers:
    Chrome: 87.0.4280.88
    Firefox: 85.0

@RIP21
Copy link
Author

RIP21 commented Feb 1, 2021

Unfortunately, there is no way to disable d.ts generation completely to replace this functionality with regular tsc into the same dist folder as tsdx does.

@RIP21
Copy link
Author

RIP21 commented Feb 1, 2021

The only workaround I found for now is
Set in tsconfig "declaration": false to make tsdx not output any d.ts
And then change scripts to similar

    "start": "tsdx watch --transpileOnly --onSuccess \"yarn typings\"",
    "build": "tsdx build --transpileOnly && yarn typings ",
    "typings": "tsc --emitDeclarationOnly --noEmit false --outDir /dist --declaration ",

Although takes a bit more time.

You can try to make a parallel run using npm-run-all or similar, but it's rather non-reliable as tsdx may delete tsc generated files.

I end-up with such scripts

    "start": "run-p tsdx:watch tsc:watch",
    "build": "run-p tsdx:build tsc",
    "tsdx:watch": "tsdx watch --transpileOnly",
    "tsdx:build": "tsdx build --transpileOnly",
    "tsc": "tsc --emitDeclarationOnly --noEmit false --outDir dist --declaration",
    "tsc:watch": "yarn tsc --watch",

Although will be nice to have it fixed :) As it definitely works improperly with PNPM.

Also, disable incremental: true if you have one, as before each build or watch tsdx will remove dist and if you have incremental only for changed files d.ts will be emitted while all others will be wiped away.

@jessekrubin
Copy link

@RIP21 What version of rush and pnpm are you using?

@RIP21
Copy link
Author

RIP21 commented Feb 16, 2021

@RIP21 What version of rush and pnpm are you using?

"rushVersion": "5.38.0"
"pnpmVersion": "5.15.2"

@jessekrubin
Copy link

@RIP21 I am using the same versions. I often get very strange type errors when building react component libs. Do you?

@RIP21
Copy link
Author

RIP21 commented Feb 17, 2021

@RIP21 I am using the same versions. I often get very strange type errors when building react component libs. Do you?

Not really. If you, by accident, have, preserveSymlinks: true in tsconfig, make sure to disable it that will solve most of the problems.
But other than that, should be fine.
Just make sure to use tsc directly and not tsdx as it handles pnpm wrongly (underlying plugin they're using more likely)

@jessekrubin
Copy link

@RIP21 Hm. Just checked and I do not have that set. All the errors I get are of the form 'propertyName' does not exist on type 'IntrisicAttributes & ... etc`

Here is an error message I get when using a Stack from @fluentui/react: Property 'tokens' does not exist on type 'IntrinsicAttributes & IStackItemProps & { children?: ReactNode; }'

I get a similar error using material-ui as well as several other react-component libs.

@jessekrubin
Copy link

@RIP21 I don't get those types of errors when building outside of rush/pnpm. I can spread in the props and it sometimes goes away but it's inconsistent

@agilgur5 agilgur5 changed the title Wrong typings generation Wrong typings generated when importing types from a package Mar 5, 2021
@agilgur5 agilgur5 changed the title Wrong typings generated when importing types from a package Wrong typings generated when importing types from a package in a Rush/PNPM repo Mar 5, 2021
@agilgur5 agilgur5 added scope: upstream Issue in upstream dependency topic: rollup-plugin-typescript2 Issues and PRs relating to rpts2 labels Mar 5, 2021
@agilgur5 agilgur5 changed the title Wrong typings generated when importing types from a package in a Rush/PNPM repo Wrong typings generated when importing types from a package in a Rush monorepo (PNPM hard links) Mar 6, 2021
@agilgur5 agilgur5 added the kind: support Asking for support with something or a specific use case label Mar 6, 2021
@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 6, 2021

So #953 reports part of the same error (the "no member" error) similarly in a Rush monorepo. It seemed like it might be due to the PNPM hard links that things break, but I haven't much of a clue why. I did a bit of debugging in that issue but really not sure where to go after those simpler leads turned up nothing. It's not clear why hard links would break things.

TSDX uses rollup-plugin-typescript2 for practically all of the TS processing, so my guess is the error is upstream there. If you're able to reproduce solely with that one plugin (I had been working on a CodeSandbox for repros for rpts2 given how frequently they happen and the amount of config TSDX wraps that can make a minimal repro difficult, but never quite got to it -- maybe I'll do that soon. EDIT: That exists on Stackblitz here now), that would narrow it down to rpts2 as the culprit and can file an upstream issue there.
There's a chance that some other of the Rollup plugins we use is responsible though, e.g. resolve or something struggling with hard links, so failure to reproduce with plain rpts2 would point in that direction instead.

rpts2 has had a number of bugs that have gone unfixed, so I've been eyeing swapping it out, but unfortunately TSDX has a history of bugs with the other 2 Rollup TS plugins as well (and rpts2 is the simpler of the bunch, so I've been able to contribute upstream several times as a result, which has not been as straightforward for the others)

@jessekrubin
Copy link

@agilgur5 what would you swap it out for? Esbuild? Some other thing?

I think you are right about pnpm being problematic.

@agilgur5
Copy link
Collaborator

agilgur5 commented Jun 10, 2022

For reference, I am fairly sure I fixed this upstream in ezolenko/rollup-plugin-typescript2#332, which fixed ezolenko/rollup-plugin-typescript2#330, which is a very similar upstream issue with pnpm symlinks.

To use that in your project, you can set your pnpm overrides to use rollup-plugin-typescript2 0.32.0+:

{
  "pnpm": {
    "overrides": {
      "rollup-plugin-typescript2": "^0.32.0"
    }
  }
}

And indeed, while the issue is different here, the root cause is the same as #953 so they are effectively duplicates.

@agilgur5 what would you swap it out for? Esbuild? Some other thing?

No, I meant a different Rollup TypeScript plugin. They all have their own issues however and I was already contributing to rpt2 a bit in the past to fix issues from TSDX. I found rpt2 to be the simplest to contribute to by a wide margin, so I actually recently stepped up to help maintain it and have fixed a ton of issues in the past month, including the most long-standing ones like this one with pnpm symlinks. Can see ezolenko/rollup-plugin-typescript2#234 (comment) for more details on that (and a root cause analysis on this and why TypeScript integrations are difficult to implement due to TS having a sparsely documented API).

@agilgur5 agilgur5 added kind: bug Something isn't working solution: duplicate This issue or pull request already exists solution: workaround available There is a workaround available for this issue and removed kind: support Asking for support with something or a specific use case labels Jun 10, 2022
@RIP21
Copy link
Author

RIP21 commented Jun 10, 2022

@agilgur5 thank you for your work! I'm no longer working on this codebase to try, but still. Tsdx is my tool to go, so I'm happy it's probably fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working scope: upstream Issue in upstream dependency solution: duplicate This issue or pull request already exists solution: workaround available There is a workaround available for this issue topic: rollup-plugin-typescript2 Issues and PRs relating to rpts2
Projects
None yet
3 participants