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

"composite: true" ignores existing .d.ts files for .js sources #47035

Closed
freshp86 opened this issue Dec 6, 2021 · 10 comments
Closed

"composite: true" ignores existing .d.ts files for .js sources #47035

freshp86 opened this issue Dec 6, 2021 · 10 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@freshp86
Copy link

freshp86 commented Dec 6, 2021

Bug Report

When using composite: true to refer from a project A to project B TypeScript generates .d.ts files for all the files in project B, even for .js files that already have user-provided d.ts files.

🔎 Search Terms

composite, definition, generate

🕗 Version & Regression Information

Reproducing on 4.2.3 (did not try older versions)

⏯ Playground Link

// @filename: foo.js
export function foo() { return null; }

// @filename: foo.d.ts
export function foo(): string;

// @composite: true

Workbench Repro

🙁 Actual behavior

TypeScript generates a new foo.d.ts file based on foo.js instead of reusing the existing foo.d.ts. In the toy example above, note that the return type of foo() is inferred from the code as null.

🙂 Expected behavior

TypeScript should reuse the existing foo.d.ts, instead of trying to infer a new one from foo.js. In the toy example above the return type of foo() should be string as is already declared in the input file foo.d.ts. Note that in a real-world example the inferred d.ts file can be incorrect causing problems when other projects try to consume it by usingcomposite: true and project references.

@RyanCavanaugh RyanCavanaugh added the Needs More Info The issue still hasn't been fully clarified label Dec 7, 2021
@RyanCavanaugh
Copy link
Member

The workbench example doesn't really show me what's going on here. Can you make a sample repo?

Note: It's unlikely you're seeing a bug here; this sounds like a misconfiguration or misunderstanding

@freshp86
Copy link
Author

freshp86 commented Dec 7, 2021

From the workbench example, switch to the "Debug" tab, scroll down to "Virtual file system", and observe the generated foo.d.ts file, see screenshot https://i.imgur.com/blG8QMO.png.

It should say export function foo(): string; (either matching the input foo.d.ts file, or being a straight copy of it), but that's not the case. I hope this helps to understand the issue, without the need for making a sample repo. Let me know if that's not the case.

@fatcerberus
Copy link

I don’t think TS uses .d.ts files to infer types in JS in general? They only provide types for the consumer of the .d.ts file.

@freshp86
Copy link
Author

freshp86 commented Dec 7, 2021

I don’t think TS uses .d.ts files to infer types in JS in general?

I think you have misunderstood the issue here. TS claims that one should be able to use composite: true and Project References to depend on another project, see [1]. That does not always work, if the other project uses a .js+.d.ts pair combo, because the .d.ts file placed in that project's output folder is incorrect (auto-generated for no reason).

In other words TS uses a .js file to generate a .d.ts file when it shouldn't, because a .d.ts file is already provided.

[1] https://www.typescriptlang.org/docs/handbook/project-references.html

@RyanCavanaugh
Copy link
Member

The workbench repro isn't great because the root cause of the problem here is that the .js file is in the project in the first place, which is the underlying misconfiguration that makes the project reference scenario not work like you're expecting.

It's never the case that we copy a .d.ts file to an output folder; that isn't a mode of operation

It's also never the case that we use a .d.ts file to ascribe typing to an "adjacent" JS file.

The way this is "supposed" to work is that the referencing project would import into the referenced project, and get redirected to that project's output .d.ts files which are autogenerated from the JS files.

I don't see a bug here unless something has specifically regressed; this behavior has been in place since 3.0 and it's unlikely you've stumbled into something no one else has noticed in the intervening years.

@freshp86
Copy link
Author

freshp86 commented Dec 8, 2021

the problem here is that the .js file is in the project in the first place,

allowJs: true allows a JS file to be part of a TS project. So not sure why having a js file as part of the project is considered incorrect here.

Perhaps I have dumbed down this minimal example more than I should, to the point where the validity of this bug is not clear. I'll create a full repo then to showcase the problem, but the end result is that a project that uses allowJs: true + composite: true + a .d.ts/.js combo is not able to be referred to by another project because TS throws errors caused by the auto-generated .d.ts file.

Note that this problem was discovered as part of TS work happening in the Chromium repository where we have a tsconfig that uses fuse.js+fuse.d.ts (from https://www.npmjs.com/package/fuse.js), and then trying to refer to that tsconfig from another tsconfig. TS generates d.ts from fuse.js which is a mangled+minimized file to begin with and results in incorrect d.ts, compared to the manually provided fuse.d.ts file. Again, I think this will become clearer with a full repro instead of a minimal example (I'll ping this thread once I have a sample repo ready).

aarongable pushed a commit to chromium/chromium that referenced this issue Dec 8, 2021
 - Add "composite: true" to c/b/r/tab_search:build_ts
 - Add workaround for
   microsoft/TypeScript#47035.
 - Pass all test files to ts_library(), while still using JS.
 - Serve tests from chrome://webui-test/
 - Move tests from browser_tests_js_webui to
   browser_tests_js_mojo_webui, since it is more appropriate.
 - Use absolute chrome://webui-test/test_util.js and
   chrome://webui-test/chai_assert.js paths.

These are in preparation of actually migrating files from JS to TS.

Bug: 1260300,1260297
Change-Id: Ib5dd46535a8d0c916973844155d2824418dc926a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3319713
Reviewed-by: Thomas Lukaszewicz <[email protected]>
Commit-Queue: Demetrios Papadopoulos <[email protected]>
Cr-Commit-Position: refs/heads/main@{#949441}
@freshp86
Copy link
Author

freshp86 commented Dec 9, 2021

@RyanCavanaugh: Please see sample repository showcasing the problem at https://github.com/freshp86/typescript-issue-47035. Repro instructions are in the README.md file.

Hopefully this makes it easier to assess the problem being reported here.

@RyanCavanaugh
Copy link
Member

OK, I see what's happening here. TL;DR this setup is not supported.

When module resolution lands on a file that is a source input of a referenced project, TypeScript uses that project's tsconfig to instead load the emitted declaration file for that source input.

In your case, that means module resolution found project_a/fuse.js (because allowJs is on), realized that it should use that file's output instead because it's a source input for a referenced project, and used project_a/tsc/fuse.d.ts instead. This is what project references are supposed to do.

If you want this to work, I think the only solution is just a general "do something differently", since the setup here is effectively tricking TypeScript into thinking that something different is happening than it expects. I'd move fuse.js into project_a/tsc and remove fuse.js from the file input list. Working example at https://github.com/RyanCavanaugh/typescript-issue-47035

@RyanCavanaugh RyanCavanaugh added Question An issue which isn't directly actionable in code and removed Needs More Info The issue still hasn't been fully clarified labels Dec 9, 2021
@freshp86
Copy link
Author

Thanks for looking into this and for the suggestions. Some comments below

TypeScript uses that project's tsconfig to instead load the emitted declaration file for that source input.

Ack. My earlier claim was that TS should not emit a new declaration file when a manual one is already provided side-by-side with the input source file. You have addressed that earlier with the following comment

It's never the case that we copy a .d.ts file to an output folder; that isn't a mode of operation

Overall it feels that composite: true is piggybacking on declaration: true, where the latter indeed has no reason to copy over an existing .d.ts file, even though it would make more sense for the former.

Note that in our real world scenario in the Chromium repo project_a is the prod code, and project_b is the test code for project_a, which I think is not that unique of a setup (having prod/test separated into different tsconfig.json files and folders)

I'd move fuse.js into project_a/tsc and remove fuse.js from the file input list.

Ack. That's very similar to what we've done so far as a workaround at [1], where

  • fuse.js was removed from the input list (tsc still copies this file over to the output folder since it is referenced by other input files)
  • fuse.d.ts is manually copied to output folder before the tsc invocation

Sounds like the workaround is the long term fix here. It still feels that this setup should be supported. Having project_b tsc execution throw an error within the source code of project_a is odd, and somewhat makes project_a leak its internal details (the fact that it uses a JS+.d.ts pair) to the outside world when it shouldn't.

Either way since there is a reasonable workaround, feel free to close this report.
Thanks again for the discussion!

[1] https://chromium-review.googlesource.com/c/chromium/src/+/3319713

@typescript-bot
Copy link
Collaborator

This issue has been marked as 'Question' and has seen no recent activity. It has been automatically closed for house-keeping purposes. If you're still waiting on a response, questions are usually better suited to stackoverflow or the TypeScript Discord community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

4 participants