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

[Case Study] Perfect TypeScript Integration #589

Open
arcanis opened this issue Nov 18, 2019 · 28 comments
Open

[Case Study] Perfect TypeScript Integration #589

arcanis opened this issue Nov 18, 2019 · 28 comments
Assignees
Labels
case study Package compatibility report

Comments

@arcanis
Copy link
Member

arcanis commented Nov 18, 2019

What package is covered by this investigations?

TypeScript

Describe the goal of the investigation

We currently support TypeScript through PnPify, but this isn't as good as a real integration - we need to emulate a whole filesystem with all its intricacies, it changes the command line (yarn pnpify tsc instead of tsc), disables some of PnP's features (such as semantic errors)...

This investigation's goal is to figure out all the blockers that prevent TypeScript from working with Yarn 2 so that we can provide a clear answer when asked what we would be needing - and potentially open issues and PRs against TypeScript to help this effort.

Investigation report


resolveModuleName / resolveTypeReferenceDirective

Those two passes already include hooks that allow to override their typical mechanism - which is what we've actually done in ts-pnp.

My main wish would be for those hooks to be builtin to TypeScript, as they aren't particularly big. To give you an idea, ts-pnp is 80 lines big with a redundant logic. The main issue is that it would involve loading the .pnp.js data in some capacity, and the TS team has historically been reluctant. I think an easy way to solve this is to simply call require('pnpapi') instead of directly requiring the .pnp.js file.

I believe this solution would work because since the pnpapi module only exists when PnP is loaded (so not when running tsc directly, and neither when running the TS that's embed within VSCode), it means that it'll be up to the user to explicitly request to use a modified environment. In the case of VSCode it means they'll have to explicitly switch to the Workspace version:

image


fileExists / directoryExists / readFile / realpath

Now that Yarn 2 is keeping the files within zip archives, accessing them from within TS requires some extra work as we would need TS to understand that accessing /foo/bar.zip/index.js would actually mean accessing /index.js within the archive /foo/bar.zip.

This part is likely the one most likely to receive pushback from the TS team, as its implementation can potentially involve adding a large logic to the TS core that may not be useful to all their users. That being said, I think a similar approach to the one detailed above could work as well: since the PnP file generated by Yarn is able to understand those zip paths, it could easily also provide an interface that could be used to implement the functions detailed above. After that, TS could implement fileExists using something similar to:

ts.effectiveFileExists = p => {
  if (!process.versions.pnp)
    return ts.fileExists(p);

  const pnp = require(`pnpapi`);
  if (!pnp.fsExtension)
    return ts.fileExists(p);

  return pnp.fsExtension.existsSync(p);
};

Similar functions might be readDirectory / getFileSize / getModifiedTime, but since they aren't in ModuleResolutionHost I'm not 100% sure that they really matter in this context.


getAutomaticTypeDirectiveNames

This function is called by createProgram in order to figure out all the @types packages that should be loaded. The problem is that getAutomaticTypeDirectiveNames calls getEffectiveTypeRoots, which in turns calls getDefaultTypeRoots, which in turns hardcodes an access to <project>/node_modules/@types. We never get the chance to change this behaviour to indicate to TS that the @types are stored in a separate location (or, in our case, multiple separate locations).

A proposal would be to add a getAutomaticTypeRoots(currentDirectory, host) to the CompilerHost interface which would be called by getEffectiveTypeRoots. By default it would just call getDefaultTypeRoots, but an environment like PnP could override it to instead do something similar to this - eliminating I/O calls and making TS faster in the process:

const pnp = require(`pnpapi`);
const locator = pnp.findPackageLocator(`${currentDirectory}/`);
const {packageDependencies} = pnp.getPackageInformation(locator);

const typeRoots = [];
for (const [name, referencish] of packageDependencies.entries()) {
  if (name.startsWith(`@types/`) && referencish !== null) {
    const dependencyLocator = pnp.toLocator(name, referencish);
    const {packageLocation} = pnp.getPackageInformation(dependencyLocator);

    typeRoots.push(packageLocation);
  }
}

getExtendsConfigPath

This function makes an hardcoded call to nodeModuleNameResolver, which prevents the host compiler from kicking in (the rational probably being that at this point the project options haven't been decided yet, and since the resolver takes them in parameter it's unclear how that should work).

I'm not sure what the TS teams would prefer - maybe a compiler host just for this purpose? That would avoid having to specify the project options as nullable, breaking existing resolveModuleName implementations.

@arcanis arcanis added the case study Package compatibility report label Nov 18, 2019
@arcanis arcanis self-assigned this Nov 18, 2019
@arcanis
Copy link
Member Author

arcanis commented Nov 19, 2019

I went ahead and started implementing it inside a fork of TypeScript, to see in more practical terms what it would entails. Some additional findings:

  • The fileExists, directoryExists, readFile, realpath methods don't actually matter! Since we operate under the assumption that PnP support is only available when TypeScript itself is running within a PnP runtime, then we also have the guarantee that fs module will see zip archives as traditional folders. Consequently, the fileExists & co functions will also see them as folders.

  • The whole implementation (including support for getExtendsConfigPath and getAutomaticTypeDirectiveNames, on top of the regular resolveModuleName) takes literally less than a hundred lines.

My current branch is here, for reference:
arcanis/TypeScript@master...arcanis:mael/pnp

@accidentaldeveloper
Copy link
Contributor

It's late and I'm a bit sleepy, so this may be a ridiculous idea or have lots of bad consequences.

If the implementation of PnP support in TS is so minimal, would it make any sense for the Yarn team to maintain and publish a fork of TS with native PnP support? This could just be a bridge to the time when TS implements it in the official package.

@arcanis
Copy link
Member Author

arcanis commented Dec 7, 2019

It's not ridiculous; to be honest, I've been wondering that as well and I don't have a full answer yet. There are various parameters:

  • I don't know how the TS team would feel about this - I know they have good intents in the way they want to handle this, so I don't want to offend them by going behind their back.

  • We would have to track every TS release. They don't make them that often, so it might not be that much of a big deal - it could probably be automated in a large measure (we could use dependabot to trigger a rebase workflow each time a new one is published).

  • There's a slippery slope argument to be made in that if everyone was doing this, then users would have to use N forks to get all the features they want - which is obviously impossible.

    • In particular, there's the ttypescript fork which adds support for third-party transformers.
  • We would have to make it clear that we will autoclose any TypeScript-related issue.

Ping @orta, does your team have some thoughts about this?

@orta
Copy link
Contributor

orta commented Dec 10, 2019

Yeah, personally, I don't see the rest of the team getting on-board with these changes until compiler plugin API makes it feasible to happen outside of the compiler itself. This is probably the right call for the short term.

@Skillz4Killz
Copy link

Skillz4Killz commented Dec 11, 2019

Hello 👋

Is it be possible to support Github repositories that are developed in Typescript? One of the issues I've recently hit is that when a library is developed with Typescript, you can not currently install it directly from the Github repository because the code first needs to be compiled. I think a perfect integration for Typescript should also compile Typescript code when installing the repo.

Example:
Suppose I create a library in Typescript, and host it on Github and publish the package as well. At some point, my library has some sort of an issue that I can push that fix to a branch on the repo which anyone can install. However, I don't want to push this fix to stable until such a time that the fix has been thoroughly tested.

At first, I thought this was a small problem I encountered but I just saw on Prettier repository today that they decided to not have Typescript because it would prevent users from installing from their Github directly.

prettier/prettier#6286

image

Let me know if this should be a separate issue.

@arcanis
Copy link
Member Author

arcanis commented Dec 11, 2019

Github repository are packed using yarn pack before being used. If they list a prepack script, Yarn will first run yarn install followed by yarn run prepack. It's during this second step that repositories have the chance to specify build scripts to run before packing the project for consumption.

The other side of the coin is that it's significantly slower on a cold cache, unfortunately.

@hassankhan
Copy link

hassankhan commented Dec 22, 2019

Hi all, would just like say that in a Yarn-Typescript integration, I'd love to see some support for Project References if Yarn Workspaces is enabled.

The plugin would, for each package, need to check dependencies and devDependencies to see if any of them can be locally resolved, then add the relevant entries into the references section of the tsconfig.json.

@arcanis
Copy link
Member Author

arcanis commented Jan 13, 2020

Update: Yarn 2 now has transparent TypeScript support. Just run yarn add typescript and everything will work out-of-the-box thanks to the compat plugin.

@accidentaldeveloper
Copy link
Contributor

Wow @arcanis , impressive work. Well done!

@huy-nguyen
Copy link

(Cross posted from Discord).

I'm checking out Yarn v2's new TypeScript plugin and ran into a problem. I'm using create-react-app's default typescript template as the sample application. What I've found is that Yarn v2 typescript works well when no workspace is involved (https://github.com/huy-nguyen/temp-yarn-v2-no-workspace-typescript-cra) but fails when workspace is used (https://github.com/huy-nguyen/temp-yarn-v2-with-worksapce-typescript-cra). The error I got in the latter case was:

Failed to compile.
undefined
TypeScript error in undefined(undefined,undefined):
Cannot find type definition file for 'jest'.  TS2688

Can anyone help me with this error? The repro steps are in the README of each repo. Yarn version is 2.0.0-rc.23.

@DylanVann
Copy link

DylanVann commented Jan 31, 2020

I have the same error as @huy-nguyen when trying to use Create React App within a workspace.

If it can be figured out it might make sense to make a e2e test for it.

Edit: Created an issue specifically for TypeScript CRA within workspaces - #876

@clemyan
Copy link
Member

clemyan commented Mar 13, 2020

The @typescript-eslint suite is having issues when integrating with TypeScript on Windows. (see typescript-eslint/typescript-eslint#1592 (comment))

tl;dr On Windows, @typescript-eslint canonicalize file paths to lowercase in order to match TypeScript's behavior, but then getPnpTypeRoots is passed a lowercased currentDirectory which pnpapi.findPackageLocator cannot resolve.

The original getDefaultTypeRoots needs a currentDirectory in order to reimplement the node resolution algorithm from the tsconfig file to find @types/*. However, since getPnpTypeRoots only runs in an PnP environment (it bails if not), can we just ignore currentDirectory and use topLevel? It will just find all @types/* packages living in the same PnP runtime.

@missing1984
Copy link
Contributor

missing1984 commented Apr 29, 2020

@clemyan currentDirectory matters in the context of a monorep that may have multiple tsconfig.json.

Besides patching the getDefaultTypeRoots path, tsserver has another code path for watching the types folder and it's currently not working under Pnp. (adding a types package will require restart tsserver.

@chrisands
Copy link

Does berry currently supports typescript paths? Trying to use "paths": { "@/*": ["packages/*"] } with workspaces and it doesn't work properly (it detects @ as package).

@arcanis
Copy link
Member Author

arcanis commented Jun 15, 2020

@chrisands I'm not sure (don't use aliases, you don't even need them!), but @/ is an invalid pattern per our standards anyway, we would recommend app/* or @company/app/* instead.

@chrisands
Copy link

Thanks @arcanis
Resolving to a package name is a great feature!

@vlazh
Copy link

vlazh commented Sep 21, 2020

How can I configure typeRoots for my ts project like this?

  "compilerOptions": {
    "typeRoots": [
      "./node_modules/@types",
      "./node_modules/some-package/types",
      "./node_modules/some-package2/types",
      "./@types"
    ],
    "types": ["app", "ts-utils", "styled-components", "webpack-env"]
  }

@merceyz
Copy link
Member

merceyz commented Mar 17, 2021

How can I configure typeRoots for my ts project like this?

@vlazh You don't, import the types directly or use https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html#-reference-types-

@vlazh
Copy link

vlazh commented Apr 9, 2021

How can I configure typeRoots for my ts project like this?

@vlazh You don't, import the types directly or use https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html#-reference-types-

triple-slash-directives (path) not working with packages in node_modules without relative or absolute path. But thanks for the direction.
I found another workaround which might work. Just create d.ts file and import types:

// src/@types/index.d.ts
import 'package1/types';
import 'package2/types';
import './app';
import './json-prune';

Make sure that file included in compilation.

@Eli-Black-Work
Copy link

@vlazh, I think you just made my day! ^_^

Although better to use export, I think:

d.ts file:

export * from './app';

@merceyz
Copy link
Member

merceyz commented Apr 9, 2021

triple-slash-directives (path) not working with packages in node_modules without relative or absolute path. But thanks for the direction.

I linked to the types section

/// <reference types="node" />

@vlazh
Copy link

vlazh commented Apr 9, 2021

I linked to the types section

It not working with types that not under node_modules/@types folder

@merceyz
Copy link
Member

merceyz commented Apr 9, 2021

@vlazh
Copy link

vlazh commented Apr 9, 2021

Although better to use export, I think:

It doesn't do anything if you have global types (no export, as in my case) and if you don't plan on importing that file.

@vlazh
Copy link

vlazh commented Apr 9, 2021

Next.js uses it for it's own package which is not under @types https://github.com/vercel/next.js/blob/0af3b526408bae26d6b3f8cab75c4229998bf7cb/examples/with-typescript/next-env.d.ts#L1-L2

As I can see, the Next package has its own ts types and next-env.d.ts does nothing. It is safe to remove it.

@merceyz
Copy link
Member

merceyz commented Apr 9, 2021

As I can see, the Next package has its own ts types and next-env.d.ts does nothing. It is safe to remove it.

It references their types so that their global types are always available even if you haven't imported from their package yet

@vlazh
Copy link

vlazh commented Apr 9, 2021

It references their types so that their global types are always available even if you haven't imported from their package yet

Oh yes, that file reference on d.ts with some module declarations and augmentations.

@vlazh
Copy link

vlazh commented Apr 9, 2021

@merceyz I have tried again with types attr and it work now (yarn 1) same as imports! I didn't know. )

/// <reference types="full package name/types" />

wincent added a commit to wincent/masochist that referenced this issue Aug 30, 2023
ie.

- from "a single Node.js loader file will be generated"
- to "a `node-modules` will be created using symlinks and hardlinks to a
  global content-addressable store"

Source: https://yarnpkg.com/configuration/yarnrc#nodeLinker

Resulting tree:

    node_modules
    ├── @jest
    │   └── globals -> ../.store/@jest-globals-npm-29.6.4-2c74983df1/node_modules/@jest/globals
    ├── @types
    │   └── node -> ../.store/@types-node-npm-20.5.7-b5e80f1922/node_modules/@types/node
    ├── jest -> .store/jest-virtual-0fd8f97523/node_modules/jest
    ├── jest-config -> .store/jest-config-virtual-4432a42ab5/node_modules/jest-config
    ├── prettier -> .store/prettier-npm-3.0.3-fced695dae/node_modules/prettier
    ├── prettier-v2-for-jest-inline-snapshots -> .store/prettier-npm-2.8.8-430828a36c/node_modules/prettier
    ├── ts-jest -> .store/ts-jest-virtual-eb33143e6d/node_modules/ts-jest
    └── typescript -> .store/typescript-patch-2e8dbfb8ab/node_modules/typescript

This moves us from two inscrutable errors that I can't solve:

    packages/common/src/__tests__/Queue-test.ts:1:36 - error TS2307: Cannot find module '@jest/globals' or its corresponding type declarations.

    1 import {describe, expect, it} from '@jest/globals';
                                         ~~~~~~~~~~~~~~~

    packages/common/src/__tests__/StringScanner-test.ts:1:48 - error TS2307: Cannot find module '@jest/globals' or its corresponding type declarations.

    1 import {beforeEach, describe, expect, it} from '@jest/globals';
                                                 ~~~~~~~~~~~~~~~

to two errors that I can solve:

    packages/lexer/src/RegExp/__tests__/normalizeCharacterClass-test.ts:1:27 - error TS2305: Module '"@jest/globals"' has no exported member 'fail'.

    1 import {describe, expect, fail, it} from '@jest/globals';
                                ~~~~

    packages/legacy/src/lexer/__tests__/Lexer-test.ts:483:14 - error TS2571: Object is of type 'unknown'.

    483       expect(onMatch.mock.calls[0][0][0]).toBe('foo');
                     ~~~~~~~~~~~~~~~~~~~~~~~~

Some related issues/PRs shed light on the long and difficult road to get
Typescript and Yarn PNP to play together:

- yarnpkg/berry#589
- microsoft/TypeScript#28289
- microsoft/TypeScript#35206

I am actually surprised that it worked as well as it did. But those last
couple errors, not sure how to solve them or even begin troubleshooting
them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
case study Package compatibility report
Projects
None yet
Development

No branches or pull requests