fix: Use tsconfig from build if exists (closes #23673)

[yusijs]

• Closes TSConfig seemingly not used when passed in via angular's build options (see #23161) #23673

User facing changelog

Change that allows passing in a default tsConfig flag in buildOptions to angularHandler

Additional details

Allow users to pass in a specific tsconfig.json (e.g to avoid collisions with jest and similar)

Steps to test

N/A

How has the user experience changed?

N/A

PR Tasks

As a quick note - I was wondering if I should add any tests for this, but seemed like such a small change it should not warrant one. However if it seems necessary, I can look into adding a test for it.

• [na] Have tests been added/updated?
• [na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
• [na] Has a PR for user-facing changes been opened in cypress-documentation?
• [na] Have API changes been updated in the type definitions?

[cypress-bot]

Thanks for taking the time to open a PR!

• Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
• Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

[CLAassistant]

All committers have signed the CLA.

[lmiller1990]

Generally we wants tests for anything - especially a regression like this.

You could modify one of the projects in system-tests/angular* to regress, than ensure it passes. Eg this one: https://github.com/cypress-io/cypress/tree/develop/system-tests/projects/angular-14.

To run that test, you would add systemTests.it.only (otherwise it'll take a long time) then do:

cd system-tests
yarn test component_testing_spec

It should run the tests.

If you could add this, it'd be great - this could be in the next release. Otherwise, it'll still get added by someone internally, but might take a little longer.

Let me know if you need any more help to get the test working.

[yusijs]

I did amend an existing test to ensure it uses the correct tsconfig.json. Can add an additional test to ensure it falls back to tsconfig.json if it is left blank.

[lmiller1990]

I could be missing something (don't know this part of the code base really well) but it looks like the test would pass either way, since if options.tsconfig is null, it'll fall back to tsconfig.json (if it exists, which it does).

Eg, if I remove your commit, will something fail?

Here's what I was thinking (just thinking out loud here):

We could copy+paste system-tests/projects/angular-14 to make a new directory (maybe system-tests/projects/angular-14-build-options).

The way the system-tests work is they look at package.json at the projectFixtureDirectory field and inherit those files (this field). If you put a different file (eg, your own tsconfig.json that's empty) that one would "take priority" and the project won't get the base copy from the angular project as specified in projectFixtureDirectory.

Then we could add an empty tsconfig.json (that won't work). To validate your change, in buildOptions in angular-14-build-options/cypress.config.ts, we'd provide one that does work.

This way, it'll use the valid one from buildOptions - then, if we did revert your commit, that test would fail (since it'd default to the empty/invalid tsconfig.json.

Let me know if 1) this makes sense (Cypress test infrastructure is pretty big and complex) or 2) I'm missing something and we don't actually need this extra test to cover the regression.

[yusijs]

1. this makes sense (Cypress test infrastructure is pretty big and complex)

It kind of makes sense, but as you say the test infra is pretty complex. If my commit is removed, nothing will stop working and tests will still pass (due to falling back to old behaviour w/ tsconfig.json to avoid regressions). It seems pretty straight forward to write a test like you mentioned though, so I can give that a spin :)

2. I'm missing something and we don't actually need this extra test to cover the regression.

So the reason I decided not to write a test originally was simply that I didn't see a value to the test. Previously there were no test to verify that the user-defined tsConfig was used, but after my changes to angularHandler.spec.ts it now verifies that it uses whichever file is passed in, but still falls back to the previous behaviour (i.e using the root tsconfig.json).

That doesn't necessarily mean we dont need a specific test for this, I just didn't see any value from it (and if Cypress has taught me anything, we should write valuable tests :) )

[ZachJW34]

Changes look good!

Would be good to modify npm/webpack-dev-server/test/handlers/angularHandler.spec.ts -> expectGeneratesTsConfig to verify that the generated tsConfig has the "extends": "<project-root>/<buildOptions.tsConfig>"

[lmiller1990]

I tried to add at test but no luck - are we sure this is working as expected? It still generated a tsconfig.json, but it didn't have my additional fields.

I did

buildOptions: {
  projectConfig: {
    tsConfig: "custom.config.json"
  }
}

My custom config had:

{
  "compilerOptions": {
    "target": "es5"
  }
}

I think in additional to adding a test, we need to document how it behaves. Maybe in npm/angular/README.md? Does it override the existing tsconfig, or merge? Also, should I pass an object of options or a path to a tsconfig.json?

[yusijs]

Eeh, it works as it should, yes. It doesnt change the generated tsconfig aside from the filename it references (since it extends it). I began writing a test myself but I never got the test to actually make any sense, since it already tests for the exact behaviour expected.

@jordanpowell88 do you have any input here (I believe you did the buildOptions themselves)?

[yusijs]

Changes look good!

Would be good to modify npm/webpack-dev-server/test/handlers/angularHandler.spec.ts -> expectGeneratesTsConfig to verify that the generated tsConfig has the "extends": "<project-root>/<buildOptions.tsConfig>"

It does, I think?

https://github.com/cypress-io/cypress/pull/23696/files#diff-59bd76d1ab6c0d9fb482c07fccbb0662ecb8cb09994a0d611f7f3b5383b36a81R198

[ZachJW34]

Here is a modified version of expectGeneratesTsConfig, it verifies the default tsConfig from the default project is extended and that a custom tsConfig is extended when passed.

const expectGeneratesTsConfig = async (devServerConfig: AngularWebpackDevServerConfig, buildOptions: any) => {
  const { projectRoot } = devServerConfig.cypressConfig
  let tsConfigPath = await generateTsConfig(devServerConfig, buildOptions)
  const tempDir = await getTempDir()

  expect(tsConfigPath).to.eq(path.join(tempDir, 'tsconfig.json'))

  let tsConfig = JSON.parse(await fs.readFile(tsConfigPath, 'utf8'))

  expect(tsConfig).to.deep.eq({
    // verifies the default `tsconfig.app.json` is extended
    extends: toPosix(path.join(projectRoot, 'tsconfig.app.json')),
    compilerOptions: {
      outDir: toPosix(path.join(projectRoot, 'out-tsc/cy')),
      allowSyntheticDefaultImports: true,
      skipLibCheck: true,
    },
    include: [
      toPosix(path.join(projectRoot, 'src/**/*.cy.ts')),
      toPosix(path.join(projectRoot, 'src/polyfills.ts')),
      toPosix(path.join(projectRoot, 'node_modules/cypress/types/index.d.ts')),
    ],
  })

  const modifiedBuildOptions = cloneDeep(buildOptions)

  delete modifiedBuildOptions.polyfills
  modifiedBuildOptions.tsConfig = 'tsconfig.cy.json'

  const modifiedDevServerConfig = cloneDeep(devServerConfig)
  const supportFile = path.join(projectRoot, 'cypress', 'support', 'component.ts')

  modifiedDevServerConfig.cypressConfig.supportFile = supportFile

  tsConfigPath = await generateTsConfig(modifiedDevServerConfig, modifiedBuildOptions)
  tsConfig = JSON.parse(await fs.readFile(tsConfigPath, 'utf8'))

  expect(tsConfig).to.deep.eq({
    // verifies the custom `tsconfig.cy.json` is extended
    extends: toPosix(path.join(projectRoot, 'tsconfig.cy.json')),
    compilerOptions: {
      outDir: toPosix(path.join(projectRoot, 'out-tsc/cy')),
      allowSyntheticDefaultImports: true,
      skipLibCheck: true,
    },
    include: [
      toPosix(path.join(projectRoot, 'src/**/*.cy.ts')),
      toPosix(supportFile),
      toPosix(path.join(projectRoot, 'node_modules/cypress/types/index.d.ts')),
    ],
  })

[lmiller1990]

Nice, let's add that test and merge this up - it can go into the next release (Tue 13th Sep).

[yusijs]

Added it in :) I was a bit worried originally about changing the existing tests to much, but makes sense like this :)

[lmiller1990]

Nice job!