Add a TypeScript CRA E2E test

[dstaley]

What's the problem this PR addresses?

The documentation lists CRA as having "Native" support for PNP, but out of the box the TypeScript version of CRA fails with errors when building.

How did you fix it?

This PR adds an E2E test that uses the TypeScript template for CRA to ensure that compatibility is maintained.

Currently, this step fails, but I'd love to also include whatever workaround is necessary to make it work so that those using TypeScript with CRA know exactly what to change to get it to work.

[arcanis]

Thanks for the test, it's helpful! I've investigated a bit and there are two problems:

• The @typescript-eslint/eslint-plugin package doesn't have an optional peer dependency on typescript (Add info about @typescript-eslint/eslint-plugin #311)

• Even after the previous problem is fixed, react-script doesn't have this optional peer dependency either so typescript still isn't made available to @typescript-eslint/eslint-plugin (peer dependencies must be declared at every level of a dependency chain).

By adding the two peer dependencies to those packages, CRA builds fine. The best fix going forward would be for both projects to add typescript to their optional peer dependencies, which is currently blocked on npm/cli#224 (comment).

[dstaley]

Should I update this PR to manually edit the lockfile so that the tests pass? Or would you prefer to wait until all the fixes land?

[arcanis]

I'd prefer to wait for the upstream fixes. As a first step, we could open PRs with the relevant projects to add optional peer dependencies and mention that:

• Optional peer dependencies work as expected with both Yarn and pnpm.

• In the case of npm, it supports it as well for subsequent installs. Initial installs stills reports a warning but that's only pending a server fix, so users won't have to upgrade to benefit from it.

• Peer dependencies' goal isn't only to print warning if they aren't fulfilled - they are a critical part of the package manager install algorithm, and have a direct effect on which hoisting patterns are allowed. Omitting them may look more pretty, but it may also cause very real breakages that will be extremely hard to debug (a package would end up requiring the wrong version of its peer dependency). The balance then becomes "printing extraneous messages on one case of one package manager" versus "breaking the install layout for all of them".

This way they can decide whether they want to merge it now, or wait for the npm fix to land on the registry.

[dstaley]

While we're waiting for this to be fixed upstream, what would be the recommended way for users to manually fix the peer dependencies? Is the best bet to manually edit yarn.lock, or is there some hook we can tap into like pnpm's readPackage hook?

[iansu]

We're happy to fix this in Create React App once the required changes are in npm and typescript-eslint.

[dstaley]

@iansu Thanks to #531, you can now add peerDependenciesMeta without affecting npm users. The same change to @typescript-eslint/eslint-plugin was published today, so now react-scripts is the last dependency in the chain preventing pnp from working with a typescript version of create-react-app.

I was going to send a PR myself, but I see there's some work going on around templates, so I didn't know which branch to target. If you'd like me to make the change, just let me know which branch to edit and I can get that submitted today. Otherwise I'll be on the lookout for a release containing the fix so we can get this E2E test merged in!

[iansu]

@dstaley That's great news! If you have time to send a PR that would be appreciated. Just make it against master.

[dstaley]

@arcanis I was hoping that today's release of create-react-app 3.3.0 would fix the remaining issues, but it seems like create-react-app is unable to determine that TypeScript is installed. The error being reported is coming from this code:

  let ts;
  try {
    ts = require(resolve.sync('typescript', {
      basedir: paths.appNodeModules,
    }));
  } catch (_) {
    console.error(
      chalk.bold.red(
        `It looks like you're trying to use TypeScript but do not have ${chalk.bold(
          'typescript'
        )} installed.`
      )
    );
    console.error(
      chalk.bold(
        'Please install',
        chalk.cyan.bold('typescript'),
        'by running',
        chalk.cyan.bold(
          isYarn ? 'yarn add typescript' : 'npm install typescript'
        ) + '.'
      )
    );
    console.error(
      chalk.bold(
        'If you are not trying to use TypeScript, please remove the ' +
          chalk.cyan('tsconfig.json') +
          ' file from your package root (and any TypeScript files).'
      )
    );
    console.error();
    process.exit(1);
  }

I'm guessing that when resolve.sync('typescript') is being called, that the pnp loader isn't active? I'm not sure if that's the case, but the test logs clearly show TypeScript being downloaded:

➤ YN0000: ┌ Fetch step
➤ YN0013: │ @types/jest@npm:24.0.23 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ @types/typescript@npm:2.0.0 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ typescript@npm:3.7.3 can't be found in the cache and will be fetched from the remote registry
➤ YN0000: └ Completed in 22.35s

[arcanis]

Has the way react-scripts is initialized changed recently? It seems like verifyTypeScriptSetup (which is in react-scripts, so should part of the newly installed project) is actually running within the create-react-app runtime itself 🤔

Basically the problem is that we have two projects with two different dependency trees (create-react-app on one side, and the newly bootstrapped project on another), and Yarn cannot reconcile the two of them (since the cache folder may be shared between the two projects we have no way to disambiguate which vendor file should access which dependency set).

This problem is the same as the one blocking #605, but unfortunately it's a tricky one. A fix would be for create-react-app to call yarn run react-scripts init to delegate the verifyTypeScriptSetup etc. This way each dependency tree would live into its own Node process, preventing ambiguities. I actually thought I had implemented this in CRA some time ago, but apparently not!

An alternative would be for Yarn to somehow detect when a file is accessed from another project, and open an isolated context with its own PnP environment. It would probably be the best option, the problem being that it will also require some decent work 🤔

[arcanis]

Fixed with #630! 🎉

[dstaley]

@arcanis are these errors just inaccurate peer dependencies errors? If so, could you let me know which packages need to be updated (I have my thoughts, but I don't want to send PRs based on my limited understanding)? I'm happy to go out and make PRs!

[arcanis]

I think it's the problem fixed by typescript-eslint/typescript-eslint#1327 🤔

That's said I wonder why I didn't see it locally when I tested last time, or why it doesn't cause the E2E to fail. Do you have an idea?

[dstaley]

I'm not too familiar with how eslint works internally, but I assume it's either because parser loading errors aren't actual errors or that CRA doesn't return an error code if eslint fails to load a parser. (My guess is that CRA tries to lint every file that it has a parser for, and simply ignores files it doesn't have a parser for.) I'm going to see if we can manually run eslint and see if that exits with an error code; if so I can update the E2E test to call that step as well.