jaredpalmer / tsdx

Zero-config CLI for TypeScript package development
https://tsdx.io
MIT License
11.22k stars 507 forks source link

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

Open RIP21 opened 3 years ago

RIP21 commented 3 years ago

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 https://github.com/formium/tsdx/issues/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

RIP21 commented 3 years ago

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

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

jessekrubin commented 3 years ago

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

RIP21 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

I think you are right about pnpm being problematic.

agilgur5 commented 2 years ago

For reference, I am fairly sure I fixed this upstream in https://github.com/ezolenko/rollup-plugin-typescript2/pull/332, which fixed https://github.com/ezolenko/rollup-plugin-typescript2/issues/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 https://github.com/ezolenko/rollup-plugin-typescript2/issues/234#issuecomment-1139202740 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).

RIP21 commented 2 years ago

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