Repository navigation
With project references, some cross-package imports of inferred types are emitted with relative paths #39117
Description
TypeScript Version: 3.9.5 (also repros with nightly)
Search Terms: project references, declaration files, tsconfig paths, inferred types
Code
The repro requires a monorepo with project references plus paths in the tsconfig. Full repro with the problematic compiled output checked in here: https://github.com/ecraig12345/learn-a/tree/inferred-relative-imports (it's fairly minimal but may still include a few settings which turn out to be tangential to the issue, sorry)
Here's a summary...
@fluentui/pkg2 depends on @fluentui/pkg1, and both live in the same monorepo under a packages folder.
pkg2's tsconfig has a project reference pointing to pkg1, and both build with composite: true.
They inherit from a shared tsconfig which has a path config like this (to allow imports directly from the root of each package):
"baseUrl": ".",
"paths": {
"@fluentui/*": ["packages/*/src"]
}
The problem comes when you have a file in one package (pkg2/src/index.ts in the repro) which has an implicit/inferred reference to a type from another local project-referenced package.
import { IThings } from '@fluentui/pkg1';
export function fn4() { // inferred return type: IThings from pkg1
const a: IThings = { thing1: { a: 'b' } };
return a.thing1;
}This inferred type need not be stated locally, but it will show up in the declaration file as an inline import().
Expected behavior:
Compile each package with tsc (or in the repro, run yarn build from the root).
pkg2's index.d.ts looks like this:
export declare function fn4(): import("@fluentui/pkg1").IThing;Actual behavior:
pkg2's index.d.ts looks like this, which will break consumers:
export declare function fn4(): import("../../pkg1/src").IThing;pkg2/src/index.ts output also adds an src folder under lib, which I wouldn't expect (pkg2/lib/src/index.{js,d.ts}), but this is probably unrelated.
Variants:
- Same behavior if
-bis added to the build command. - Same behavior if using
rootDirinstead ofinclude, just different output directory structure. - Same behavior if the config inheritance is removed and
pathsconfig is moved to each project. - Without project references, you end up with a directory structure like this, which I also don't understand, but is not the same issue:
pkg2/
lib/
pkg1/src/<files>
pkg2/src/<files>
Playground Link: can't be shown on playground
Related Issues: didn't see anything else quite like this
Activity
robpalme commentedon Jun 23, 2020
Hello Elizabeth Craig (@ecraig12345), this is a duplicate of #38111
I wrote up our solution there. The summary is that when wiring up dependencies,
"paths"currently won't give you what you want. Ambient module declarations are the only way to avoid the issue you describe.It would be awesome if
"paths"had the same super-power as ambient module declarations, because we made the same incorrect assumption that"paths"would give us bare-specifier linkage for free.daniele-orlando commentedon Aug 4, 2020
I can reproduce this bug after upgrading TypeScript from version
3.7to3.9.7.I have a monorepo with:
packages/std-reactusingpackages/std-webwhich is symlinked insidepackages/std-react/node_modules.Building
packages/std-reactthrows this errorbut if I copy and paste
packages/std-webinsidepackages/std-react/node_modules, the error goes away.If import is used in the file, prefer that import specifier over calc…
sheetalkamat commentedon Jan 5, 2021
Inferred module specifiers in d.ts file are suppose to be relative so this current behavior is correct but we could prefer the existing import in the file if it refers to same file and i have a PR #42224 to try that out and see how it works out
ecraig12345 commentedon Jan 6, 2021
How is this correct if the module is in a different project? Relative inferred module specifiers are a huge problem for consumers of our .d.ts files because it's possible/likely that they just flat-out won't work if the folder structure in the consumer's node_modules is different than our monorepo's internal folder structure. Also, there are other existing cases where the inferred specifier is NOT relative and includes the package name; it's only when you mix in project references that you get the relative inferred specifiers.
sheetalkamat commentedon Jan 6, 2021
This has nothing to do with project references but path mapping provided. If you remove reference to project, you are going to get same specifier. #26341 made sure that declaration emit uses relative paths all the time. If the paths are in
node_modules, then and then only they use package name. Path mapping is not good enough indicator to use package name.ecraig12345 commentedon Jan 6, 2021
Then what is the suggested way to handle this case, where we have project references as well as deep imports into other projects?
robpalme commentedon Jan 6, 2021
Sheetal Nandi (@sheetalkamat) 's fix in #42224 looks very good. It helps make declaration emit more intuitive and more controllable by the user, because it steers towards reflecting what the user wrote. It permits adhoc fix-ups by users because, worst case, if a synthetic (inferred) import is in an undesirable form, the user can now add their own preferred alias to the source code to tweak the declaration emit.
So it's a big step forward (and I think worth landing), but it would be awesome if we could later come up with a comprehensive solution.
I understand that historically this has been a simple heuristic to determine if a file belongs to an external dependency or not. In future it would be good to avoid relying on this assumption because whilst it's a very popular pattern,
node_modulesis not a fundamental property of the language. There are other JS systems out there that unpack dependencies into other locations. And, even more common, there are monorepos where packages live side-by-side inside a directory that is not callednode_modules.In the system I work on, we solve this problem automatically for our users by post-processing the DTS files emitted by TypeScript to mop up any unwanted synthetic relative paths by converting them into a canonical form, i.e. package-prefixed bare specifiers. It works, but it sounds like Elizabeth Craig (@ecraig12345) has a similar desire, so maybe it's something that could be solved in TypeScript for everyone.
For use-cases where dependencies do not live inside
node_modules, import maps (as a concept) are one way to express the names and locations of dependencies. Which superficially looks similar to"paths"in tsconfig. This is probably why confusion arises - because when you survey tsconfig to answer the question of "how can I inform TypeScript about where to find my dependencies", "paths" seems to be the most obvious solution.I think the simplest solution would be for declaration emit to read
"paths"and use it to drive the preferred specifier selection. That's effectively what our post-processing transform achieves. It would be fine for this preference to have a lower precedence "reuse already imported specifiers" that you have implemented so far. Please say if you know of problems with this approach.use project relative preference for declaration emit
sheetalkamat commentedon Jan 6, 2021
#42232 might help with project boundary issues but will have to see how it performs given we have so many different ways the file can be referenced. Will see how tests are running to get more idea. It doesnt break test case from #26341 for sure so thats a good start.
If import is used in the file, prefer that import specifier over calc…
Philipp91 commentedon Jan 9, 2021
Thank you! I would like to test this. Normally the nightly releases appear by 8am UTC, but
4.2.0-dev.20210109hasn't appeared yet. Anyway I'll just wait until there's a nightly release and then test it for our use case.ecraig12345 commentedon Jan 11, 2021
I don't think this issue should be closed--while #42224 fixed the issue in some cases, it won't fix it in cases where the file isn't referenced in another import.
Philipp91 commentedon Jan 11, 2021
+1 to Elizabeth Craig (@ecraig12345)'s point. E.g. mui/material-ui#24112 is one of the cases that isn't solved because the symbol is only being used implicitly in the
tsxfile, so there's no explicit import that the heuristic can piggy-back on.Rob Palmer (@robpalme)
That sounds like a decent solution, as I imagine it breaks relatively rarely. Could you elaborate on how that post-processing works? Just some clever
sedcommands? Maybe you can share relevant snippets of your implementation, even if it's very specific to that project, so that others can adapt it?eps1lon commentedon Jan 13, 2021
Sheetal Nandi (@sheetalkamat) Could you re-open this issue since #42232 did not fix all issues?
I can confirm that some problematic relative import paths are removed but some are still inserted.
Full diff of generated declaration files between
4.1.3and4.2.0-dev.20210112: https://app.circleci.com/pipelines/github/mui-org/material-ui/34050/workflows/861b9ce6-4157-442d-925c-e09f67a21790/jobs/213656/parallel-runs/0/steps/0-110Example problematic import that is still generated:
import("../../../material-ui-styles/src").ClassNameMap<PickersYearClassKey>;sheetalkamat commentedon Jan 14, 2021
Can you please open a new issue with the small repro that doesnt get fixed so we can reason about it as part of #42232
eps1lon commentedon Jan 14, 2021
Sheetal Nandi (@sheetalkamat) I've seen that you can build an installable version of TypeScript from a PR. If you could do that I can quickly check it on our repo. A small repro might take longer.
eps1lon commentedon Jan 14, 2021
Sheetal Nandi (@sheetalkamat) Repro was quicker than expected: #42349
If import is used in the file, prefer that import specifier over calc…
use project relative preference for declaration emit
Use project relative preference for declaration emit (#42232)