[Bug] @yarnpkg/core definitions uses ../../yarnpkg-fslib/sources

[christophehurpeau]

• I'd be willing to implement a fix

Describe the bug

I'm trying to implement yarn 2 in renovate: renovatebot/renovate#6045
Importing @yarnpkg/core throws these errors:
##[error]node_modules/@yarnpkg/core/lib/MultiFetcher.d.ts(7,64): error TS2307: Cannot find module '../../yarnpkg-fslib/sources'.
##[error]node_modules/@yarnpkg/core/lib/VirtualFetcher.d.ts(6,64): error TS2307: Cannot find module '../../yarnpkg-fslib/sources'.
##[error]node_modules/@yarnpkg/core/lib/VirtualFetcher.d.ts(8,35): error TS2307: Cannot find module '../../yarnpkg-fslib/sources'.
##[error]node_modules/@yarnpkg/core/lib/VirtualFetcher.d.ts(10,28): error TS2307: Cannot find module '../../yarnpkg-fslib/sources'.
##[error]node_modules/@yarnpkg/core/lib/VirtualFetcher.d.ts(11,28): error TS2307: Cannot find module '../../yarnpkg-fslib/sources'.
##[error]node_modules/@yarnpkg/core/lib/VirtualFetcher.d.ts(15,50): error TS2307: Cannot find module '../../yarnpkg-fslib/sources'.
##[error]node_modules/@yarnpkg/core/lib/structUtils.d.ts(91,66): error TS2307: Cannot find module '../../yarnpkg-fslib/sources'.

You can also see the build here: https://github.com/renovatebot/renovate/pull/6045/checks?check_run_id=636048215

To Reproduce

The minimal information needed to reproduce your issue (ideally a package.json with a single dep). Note that bugs without minimal reproductions might be closed.

IMPORTANT: We strongly prefer reproductions that use Sherlock. Please check our documentation for more information: https://yarnpkg.com/advanced/sherlock

No reproduction, sorry

[arcanis]

I'll take a look - I made a release yesterday, and I think something broke in the build pipeline.

[arcanis]

Everything's fine for the build - seems like TS always built bogus TS .d.ts files, we will need to investigate more in depth 🙁

[christophehurpeau]

@arcanis thank you for looking into this :)

[regevbr]

I experience the same issue as well at snyk/nodejs-lockfile-parser#57 @arcanis please let me know if it is something you think I can fix and I will get on it.

[arcanis]

Yep it's a really annoying one 🙁 I'm not exactly sure what happens, but I suspect it's because TypeScript doesn't seen node_modules in the path, so it uses a relative path.

I was wondering if it could be solved by adding a paths configuration in our configuration script to explicitly map the packages to their names, but I haven't had the chance to try it yet. If you could give it a look I would greatly appreciate!

[regevbr]

Sure I will give it a shot later today

[regevbr]

Trying to play with paths in tsconfig doesn't work...
The strange thing is that start of file imports work correctly, but inferred types are relative. The inferred type occur when you don't declare the return type of a function, so it infers it and puts it in the d.ts file and only there it uses the relative path.
Investigation is still undergoing but I don't have much hope at this time.
Will update if I find something else

[regevbr]

According to microsoft/TypeScript#36097 (comment) the inline imports is an intended behavior to avoid name conflicts and it makes sense. This makes me think we might need to patch typescript in order to resolve this. @arcanis what do you think? I will try to see if I can find where this goes wrong in the typescript code
I also believe that this will cause issues to anyone who uses workspaces with yarn2, making this bug more critical than meets the eye...

[arcanis]

According to microsoft/TypeScript#36097 (comment) the inline imports is an intended behavior to avoid name conflicts and it makes sense

Inline imports make sense, but why would it use a relative path? That's the part I don't quite understand, since the autofix suggestions for undefined symbols are able to properly suggest the right import strings. It seems like TS has a bug that prevents it from using the package name 🤔

[regevbr]

@arcanis I will need to verify if that issue happens in yarn1 with workspaces. If it does, then it is a ts bug, otherwise it means that the issue is caused by pnpify and we need to patch it manually to resolve it

[regevbr]

This is indeed a bug in typescript.

I created the following repo to reproduce the bug - https://github.com/regevbr/typescript-workspaces-bug-reproduction-
It uses yarn1 with lerna workspaces and implicit return value that comes from a different package in the workspace.

@arcanis would you mind opening an issue in typescript? I know you have your reputation there and they will take it more seriously under consideration.

In any case the "workaround" is to explicitly state the return value of functions. I say it is a "workaround" because I think it is best practice to state the return value, rendering any change to the return value to throw a type error, causing the coder to think twice if the change of behavior is safe and compatible to any users of the function - changing a function's return value is like breaking an api...

[regevbr]

Actually I didn't notice that I used old typescript (I used another repo as the base to the repo) ... Using the latest typescript doesn't reproduce the issue... Sorry for the mess.

I do believe now that the issue comes from yarn2 pnpify due to the usage of the :workspace protocol and the lack of node_modules like @arcanis suggested to begin with. In yarn1, workspaces creates a symlink in node modules for all the packages and ts properly uses them for the generated d.ts files.

So back to the drawing boards to investigate the patched typescript cods in yarn2 :-(

[regevbr]

Ok I found the solution!
using ts 3.9.5, in typescript/lib/tsc.js:86507 there is a function called tryGetModuleNameAsNodeModule which resolves the package name from it's location.
Since there is no node_modules in the path of a workspace package, the function returns a relative path.
We need to change the function and add a new one so it will look something like:

          function getPnpWorkspaceModuleName(moduleFileName) {
            if (!isPnpAvailable()) {
              return undefined;
            }
            var pnpApi = require("pnpapi");
            var ownerPackage = pnpApi.findPackageLocator(moduleFileName);
            if (!ownerPackage || ownerPackage.reference.indexOf('workspace:') !== 0) {
              return undefined;
            }
            return ownerPackage.name;
          }
          function tryGetModuleNameAsNodeModule(moduleFileName, _a, host, options, packageNameOnly) {
            var getCanonicalFileName = _a.getCanonicalFileName, sourceDirectory = _a.sourceDirectory;
            if (!host.fileExists || !host.readFile) {
              return undefined;
            }
            var parts = getNodeModulePathParts(moduleFileName);
            if (!parts) {
              var pnpModuleName = getPnpWorkspaceModuleName(moduleFileName);
              return pnpModuleName || undefined;
            }
....

The bug is reproducible at https://github.com/regevbr/typescript-workspaces-bug-reproduction-
And the fix is verified on it.
@arcanis can you please review the fix and create a patch for it?

I'm sure it is not an airtight solution due to future plugins that can cause the same issue, so maybe you know a better way to realize that a file is part of the workspace or not?

[regevbr]

I realized the solution I provided will not work in case the import is nested, i.e @space/pkg1/dir1/dir2 and indeed it doesn't.
So a better solution will be

      function mockPnpWorkspaceNodeModule(moduleFileName) {
        if (!isPnpAvailable()) {
          return moduleFileName;
        }
        var pnpApi = require("pnpapi");
        var ownerPackage = pnpApi.findPackageLocator(moduleFileName);
        if (!ownerPackage || ownerPackage.reference.indexOf('workspace:') !== 0) {
          return moduleFileName;
        }
        var pkgDirectory = ownerPackage.reference.replace('workspace:','');
        return moduleFileName.replace('/' + pkgDirectory,ts.nodeModulesPathPart + ownerPackage.name);
      }
      function tryGetModuleNameAsNodeModule(moduleFileName, _a, host, options, packageNameOnly) {
        var getCanonicalFileName = _a.getCanonicalFileName, sourceDirectory = _a.sourceDirectory;
        if (!host.fileExists || !host.readFile) {
          return undefined;
        }
        moduleFileName = mockPnpWorkspaceNodeModule(moduleFileName)
        var parts = getNodeModulePathParts(moduleFileName);
        if (!parts) {
          return undefined;
        }

So I basically manipulate the path of the package to include node_modules and the actual package name. This works great but causes the following d.ts:

export declare const add: (a: number, b: number) => import("@ts-test-bug/pkg1/dist").Test;
export declare const addNested: (a: number, b: number) => import("@ts-test-bug/pkg1/dist/add3").Test3;

For the first line there is no need to add the /dist part as it is redundant.
I couldn't find a solution to overcome it but it is a progress.
@arcanis do you think you can take it from here? this feels way above my pay grade :-)

I have updated the reproduction repo accordingly...

[regevbr]

So to fix the issue in my previous comment, I came up with this solution, which is more complicated :-(

      var workspaceProtocol = 'workspace:';
      function mockPnpWorkspaceNodeModule(moduleFileName) {
        if (!isPnpAvailable()) {
          return undefined;
        }
        var pnpApi = require("pnpapi");
        var ownerPackage = pnpApi.findPackageLocator(moduleFileName);
        if (!ownerPackage || ownerPackage.reference.indexOf(workspaceProtocol) !== 0) {
          return undefined;
        }
        var pkgDirectory = ownerPackage.reference.replace(workspaceProtocol,'');
        var replacePartFrom = ts.directorySeparator + pkgDirectory;
        var replacePartTo = ts.nodeModulesPathPart + ownerPackage.name;
        var mockedFileName = moduleFileName.replace(replacePartFrom, replacePartTo);
        var parts = getNodeModulePathParts(mockedFileName);
        if (!parts) {
          return undefined;
        }
        var buffer = replacePartFrom.length - replacePartTo.length;
        parts.topLevelPackageNameIndex -= ts.nodeModulesPathPart.length - 1;
        parts.packageRootIndex += buffer;
        parts.fileNameIndex += buffer;
        var pnpPkgNameReplacer = function (path) {
          return path.replace(pkgDirectory, ownerPackage.name);
        }
        return {
          pnpPkgNameReplacer: pnpPkgNameReplacer,
          parts: parts,
        }
      }
      function tryGetModuleNameAsNodeModule(moduleFileName, _a, host, options, packageNameOnly) {
        var getCanonicalFileName = _a.getCanonicalFileName, sourceDirectory = _a.sourceDirectory;
        if (!host.fileExists || !host.readFile) {
          return undefined;
        }
        var pnpPkgNameReplacer;
        var parts = getNodeModulePathParts(moduleFileName);
        if (!parts) {
          var mockedParts = mockPnpWorkspaceNodeModule(moduleFileName);
          if (!mockedParts) {
            return undefined;
          }
          parts = mockedParts.parts;
          pnpPkgNameReplacer = mockedParts.pnpPkgNameReplacer;
        }
            var moduleSpecifier = moduleFileName;
            if (!packageNameOnly) {
                var packageRootIndex = parts.packageRootIndex;
                var moduleFileNameForExtensionless = void 0;
                while (true) {
                    var _b = tryDirectoryWithPackageJson(packageRootIndex), moduleFileToTry = _b.moduleFileToTry, packageRootPath = _b.packageRootPath;
                    if (packageRootPath) {
                        moduleSpecifier = packageRootPath;
                        break;
                    }
                    if (!moduleFileNameForExtensionless)
                        moduleFileNameForExtensionless = moduleFileToTry;
                    packageRootIndex = moduleFileName.indexOf(ts.directorySeparator, packageRootIndex + 1);
                    if (packageRootIndex === -1) {
                        moduleSpecifier = getExtensionlessFileName(moduleFileNameForExtensionless);
                        break;
                    }
                }
            }
            var globalTypingsCacheLocation = host.getGlobalTypingsCacheLocation && host.getGlobalTypingsCacheLocation();
            var pathToTopLevelNodeModules = getCanonicalFileName(moduleSpecifier.substring(0, parts.topLevelNodeModulesIndex));
            if (!isPnpAvailable() && !(ts.startsWith(sourceDirectory, pathToTopLevelNodeModules) || globalTypingsCacheLocation && ts.startsWith(getCanonicalFileName(globalTypingsCacheLocation), pathToTopLevelNodeModules))) {
                return undefined;
            }
            var nodeModulesDirectoryName = moduleSpecifier.substring(parts.topLevelPackageNameIndex + 1);
            if (pnpPkgNameReplacer) {
              nodeModulesDirectoryName = pnpPkgNameReplacer(nodeModulesDirectoryName);
            }
            var packageName = ts.getPackageNameFromTypesPackageName(nodeModulesDirectoryName);
...
}

The introduced mockPnpWorkspaceNodeModule function is used to calculate the indexes of parts in the file name, that are normally relative to node_modules which doesn't exist here, by "mocking" the node_modules dir into the path, and then correcting the indexes.
Using this method will allow the tryGetModuleNameAsNodeModule function to properly go over the file path and making the various calculations. At the end of it, we just need to replace the location of the package in the repo, with its actual npm name, by calling pnpPkgNameReplacer.

[regevbr]

@arcanis can you please check out my solution? I think the issue might affect a lot of projects...

[arcanis]

Hi @regevbr! Thanks a lot for investigating - I was AFK this week-end and only saw the mail notifications; I'll take a closer look at it tomorrow and report back. If I understand correctly:

• We now know how to convert /packages/foo into foo
• But not yet how to convert /packages/foo/src/baz.ts into foo/src/baz.ts

Is that correct? If so, I wonder if perhaps it's not just a matter of obtaining the locator (same as you currently do), then find the workspace location (using getPackageInformations().packageLocation), then derive the subpath by using path.relative(packageLocation, moduleFileName).

[regevbr]

The most recent post offers a complete solution to both cases!

I created various scenarios in https://github.com/regevbr/typescript-workspaces-bug-reproduction- which reproduces the issues and my fix, solves all of them.

I'm not sure what your suggestion means but I'm sure that you will be able to find a more clean and robust solution based on what I found, as I'm not familiar with the pnpify api.

To sum up the bug -
When a return value of a function is implicit, typescript needs to realize it (which it already does perfectly) and then it need to import the type in the d.ts file. Since type collision can occur, the import in those cases is being inlined in the function signature. The mechanism to realize how to create the import, is being handled at tryGetModuleNameAsNodeModule and it relies on using node_modules to realize the package name and sub path in it to use.
Since we use pnp, there is no node_modules in the inner packages paths (it does exist in the external zip files in .cache) so it creates a relative paths instead, which won't work when the package is being published.

To sum up the solution -
The new mockPnpWorkspaceNodeModule function is used to calculate the indexes of parts in the file name, that are normally relative to node_modules which doesn't exist here, by "mocking" the node_modules dir into the path, and then correcting the indexes.
Using this method will allow the tryGetModuleNameAsNodeModule function to properly go over the file path and making the various calculations. At the end of it, we just need to replace the location of the package in the repo, with its actual npm name, by calling pnpPkgNameReplacer.

Please let me know if I wasn't clear enough :) and thanks for looking into it!

[regevbr]

Hi @arcanis is there anything else I can do to help?

[arcanis]

I'm actually looking at it right now! 😃

[regevbr]

haha, if you want to DM with me about it, let me know, I'm working on a PR for another project so I'm free to help during

[arcanis]

I've a small patch that gets the following result:

import { Fetcher, FetchOptions, MinimalFetchOptions } from './Fetcher';
import { Locator } from './types';
export declare class MultiFetcher implements Fetcher {
    private readonly fetchers;
    constructor(fetchers: Array<Fetcher>);
    supports(locator: Locator, opts: MinimalFetchOptions): boolean;
    getLocalPath(locator: Locator, opts: FetchOptions): import("@yarnpkg/fslib/sources/path.ts").PortablePath | null;
    fetch(locator: Locator, opts: FetchOptions): Promise<import("@yarnpkg/core/sources/Fetcher.ts").FetchResult>;
    private tryFetcher;
    private getFetcher;
}

However, the output references the sources directory (which makes sense, since those are the files that get built) instead of what will really be there once the packages are built: the lib directory (and the .d.ts extension). Did you observe the same behaviour? I wonder how that could be fixed, since TS has no way to know about the future path translation 🤔

[regevbr]

Let me check real quick on my reproduction repo

[regevbr]

Nope for me it works great, you can see it in action in my repo https://github.com/regevbr/typescript-workspaces-bug-reproduction-

[regevbr]

Oh I know what you are tyring to do here... It will not work, you must let the rest of the method to run as there are some calculations there. That was the idea of my first try to solve

[regevbr]

thanks @arcanis!!! when can we expect a new version so we can test in other repos?

[arcanis]

The fix has been released: https://cdn.jsdelivr.net/npm/@yarnpkg/[email protected]/lib/MultiFetcher.d.ts

[viceice]

This bug is back in v3.5.0 😕

• https://cdn.jsdelivr.net/npm/@yarnpkg/[email protected]/lib/MultiFetcher.d.ts
• https://app.renovatebot.com/package-diff?name=@yarnpkg%2fcore&from=3.4.0&to=3.5.0
• build(deps): update dependency @yarnpkg/core to v3.5.0 renovatebot/renovate#21116

[merceyz]

@viceice see #5346