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

Disable composite for type declarations #54

Closed
wants to merge 2 commits into from
Closed

Disable composite for type declarations #54

wants to merge 2 commits into from

Conversation

halostatue
Copy link
Contributor

@halostatue halostatue commented Mar 1, 2024

Disable composite for type declarations

We are using pkgroll with Typescript monorepos (managed by pnpm workspaces) and project references. The latter requires compilerOptions.composite = true in referenced projects for most tsc invocations. If you set references without compilerOptions.composite = true, tsc will fail, indicating that composite needs to be set.

When I run pkgroll and have composite set, I get TS6307 for every file included from src/index.ts. rollup-plugin-dts does not handle composite (see Swatinem/rollup-plugin-dts#127, which was closed without fix or explanation). I looked at several other related tools (rollup-plugin-typescript2, rollup-plugin-ts, others mentioned in Swatinem/rollup-plugin-dts) and could not get any of them working to build just the DTS files (rollup-plugin-typescript2 was the most promising, but I could not get it working in the time that I allotted for this investigation).

Going back to rollup-plugin-dts, we find the compilerOptions option, and I tried explicitly setting composite: false, which allows my type files to build again without errors.

I don't think that this is the correct fix; ideally, rollup-plugin-dts would be fixed to better handle composite projects or would override composite at all times. An alternative change, although much more invasive to pkgroll, would be to have pkgroll accept an alternate tsconfig.json as a parameter:

$ pkgroll --tsconfig tsconfig.pkgroll.json

In the short term, this has eliminated the build issue that I have been seeing, and it may be sufficient to resolve this until a better fix can be determined in some way, whether by a --tsconfig parameter, upstream fixes, or changing to a different bundler plugin for type declaration plugins.

This issue can be seen at:

https://github.com/halostatue/pkgroll-monorepo-types-issue

@privatenumber
Copy link
Owner

Thanks for the PR!

Would you mind adding a test to demonstrate the issue you're addressing?

@halostatue
Copy link
Contributor Author

Thanks for the PR!

Would you mind adding a test to demonstrate the issue you're addressing?

I wouldn't mind, but it’s going to be at least couple of weeks before I can get to it, because I discovered it in a multi-thousands-of-lines project during refactoring — and the project is sadly very late (it was supposed to be done mid-February).

I’m also not entirely sure how the test would work with your setup, because it requires a typescript monorepo project, so as I understand it, it would be an entirely separate fixture layout for this test.

We are using pkgroll with Typescript monorepos (managed by pnpm
workspaces) and project references. The latter requires
`compilerOptions.composite = true` in referenced projects for most `tsc`
invocations. If you set `references` without `compilerOptions.composite
= true`, `tsc` will fail, indicating that `composite` needs to be set.

When I run `pkgroll` and have `composite` set, I get TS6307 for every
file included from `src/index.ts`. rollup-plugin-dts does not handle
`composite` (see Swatinem/rollup-plugin-dts#127, which was closed
without fix or explanation). I looked at several other related tools
(rollup-plugin-typescript2, rollup-plugin-ts, others mentioned in
Swatinem/rollup-plugin-dts) and could not get any of them working to
build *just* the DTS files (rollup-plugin-typescript2 was the most
promising, but I could not get it working in the time that I allotted
for this investigation).

Going back to rollup-plugin-dts, we find the `compilerOptions` option,
and I tried explicitly setting `composite: false`, which allows my type
files to build again without errors.

I don't think that this is the *correct* fix; ideally, rollup-plugin-dts
would be fixed to better handle composite projects *or* would override
`composite` at all times. An alternative change, although much more
invasive to pkgroll, would be to have pkgroll accept an *alternate*
`tsconfig.json` as a parameter:

```console
$ pkgroll --tsconfig tsconfig.pkgroll.json
```

In the short term, this has eliminated the build issue that I have been
seeing, and it may be sufficient to resolve this until a better fix can
be determined in some way, whether by a `--tsconfig` parameter, upstream
fixes, or changing to a different bundler plugin for type declaration
plugins.

This issue can be seen at:

https://github.com/halostatue/pkgroll-monorepo-types-issue
halostatue added a commit to halostatue/pkgroll-monorepo-types-issue that referenced this pull request Mar 5, 2024
This repository demonstrates the issues in Swatinem/rollup-plugin-dts
(Swatinem/rollup-plugin-dts#127) with `composite` Typescript monorepos.
When using pkgroll, this will be fixed by privatenumber/pkgroll#54.

See the README.md for more information.
@halostatue
Copy link
Contributor Author

halostatue commented Mar 5, 2024

@privatenumber I managed to figure out a simple reproduction and have updated the main description with a link to the repo.

I have also added a unit test and fixture that covers this. It's a non-trivial case:

  1. You must be exporting types and bundling the resulting .d.{,m,c}ts files.
  2. You must have more than one file exporting types in src/, but everything is exported through the main file. That is, src/index.ts can simply export type { Name } from './other.js', which is exported from src/other.ts.
  3. The tsconfig.json must have compilerOptions.composite: true.

I will reiterate that I believe that this change is a hack and is fundamentally incorrect (and probably should not work, but so far it seems to). I believe that either Swatinem/rollup-plugin-dts needs to be fixed or pkgroll should look at using rollup-plugin-typescript2 for type exporting (it looks like it can be configured just for type exports, and it looks like while there were similar bugs related to composite projects, they seem to have been squashed).

I don't understand enough of what Swatinem/rollup-plugin-dts is doing to make a suitable bug report there, but hopefully the repro repo will help you understand it so that the underlying bug can be fixed, not just closed.

BTW, without the code change, this is the error reported from the unit test:

src/index.ts(1,22): error TS6307: File '/private/var/folders/c6/k_j3j3w11590l8x6t8rwxw200000gn/T/fs-fixture-1709648068713-1/packages/one/src/name.ts' is not listed within the file list of project ''. Projects must list all files or use an 'include' pattern.\n

@privatenumber
Copy link
Owner

privatenumber commented Mar 7, 2024

I'm unable to push to your PR (your repo) all of a sudden, so I pushed here:
#55 Commit history is still preserved.

git push git@github.com:KineticCafe/pkgroll.git fake-support-composite
ERROR: Permission to KineticCafe/pkgroll.git denied to privatenumber.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

I consolidated the fixture and made it more minimal.

@halostatue halostatue deleted the fake-support-composite branch March 7, 2024 15:04
@halostatue
Copy link
Contributor Author

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants