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

fix(webpack-dev-server): add typeRoots to generated tsconfig for angular #27117

Merged
merged 25 commits into from
Jul 18, 2023

Conversation

jordanpowell88
Copy link
Collaborator

@jordanpowell88 jordanpowell88 commented Jun 22, 2023

Additional details

Currently in order to generate a webpack config for Angular Component Testing we pass configuration options from the angular.json file or the devServer.options and a tsconfig file to the angular compiler. Currently we generate a new tsconfig.json file and writes to a temp directory on your machine every time the anglerHandler is called. Because the generated tsconfig file is located in a temp directory types from the extended tsconfig.app.json file are unable to resolve without defining typeRoots. This PR adds the project root typeRoots to the generated tsconfig.

Steps to test

  1. Run yarn dev and open an angular project that has a type in to the tsconfig.app.json types array.
  2. Launch Component Testing Launch
  3. Validate that no type errors are outputted in the terminal like below:
    Screenshot 2023-07-07 at 10 21 38 AM

How has the user experience changed?

PR Tasks

@jordanpowell88 jordanpowell88 changed the title feat(webpack-dev-server): generate a local tsconfig file feat(webpack-dev-server): generate a local tsconfig file for Angular Jun 22, 2023
@jordanpowell88 jordanpowell88 marked this pull request as ready for review June 23, 2023 12:43
@cypress
Copy link

cypress bot commented Jun 23, 2023

6 flaky tests on run #48898 ↗︎

0 4447 944 0 Flakiness 6

Details:

remove node_modules/types from assertions
Project: cypress Commit: 1434534c88
Status: Passed Duration: 14:53 💡
Started: Jul 17, 2023 7:46 PM Ended: Jul 17, 2023 8:01 PM
Flakiness  commands/net_stubbing.cy.ts • 3 flaky tests • 5x-driver-webkit

View Output Video

Test Artifacts
network stubbing > intercepting request > can delay and throttle a StaticResponse Output Video
... > with `times` > only uses each handler N times Output Video
... > stops waiting when an xhr request is canceled Output Video
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-webkit

View Output Video

Test Artifacts
... > correctly returns currentRetry Output Video
... > correctly returns currentRetry Output Video
... > correctly returns currentRetry Output Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like two things are happening here:

  1. the actual fix, which is to add types: ["cypress"] to the tsconfig.json
  2. write the tsconfig.cypress.json to the local repo

In the interest of getting this fixed asap, I wonder if we want a PR that just adds the fix, not any changes to the behavior?

My concern with the change is the tsconfig.json we generate will contain hard coded paths, which include the user's username:

{
  "extends": "/Users/lachlanmiller/code/dump/angular-app/tsconfig.app.json",
  "compilerOptions": {
    "outDir": "/Users/lachlanmiller/code/dump/angular-app/out-tsc/cy",
    "allowSyntheticDefaultImports": true,
    "skipLibCheck": true,
    "types": [
      "cypress"
    ]
  },
  "include": [
    "/Users/lachlanmiller/code/dump/angular-app/**/*.cy.ts",
    "/Users/lachlanmiller/code/dump/angular-app/cypress/support/component.ts"
  ]

Which means the committed files will only work on the original developer's machine. This is a blocker.

Happy to work towards having a local file, but we will need to fix this issue. In the meantime, how about a separate patch that addresses the core issue, and figure out the path issue separately (unless you know a fix right now).

@@ -256,11 +254,11 @@ const expectGeneratesTsConfig = async (devServerConfig: AngularWebpackDevServerC
outDir: toPosix(path.join(projectRoot, 'out-tsc/cy')),
allowSyntheticDefaultImports: true,
skipLibCheck: true,
types: ['cypress'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just to confirm, this is the fix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that is not the fix. The issue is because we are using a temp file for the tsconfig any types that get added to the extended tsconfig types array aren't able to resolve. Currently we manually add the Cypress types to the includes array. But with this option we don't need to do this and we just just cypress to that types array.

We could potentially resolve those paths sourcing typeRoots from the extended tsconfig but it still doesn't allow users to make any additional changes to the tsconfig. Having a tsconfig.cypress.json follows the same ergonomics of a typical Angular project tsconfig.app.json, tsconfig.spec.json, etc

@jordanpowell88
Copy link
Collaborator Author

It looks like two things are happening here:

  1. the actual fix, which is to add types: ["cypress"] to the tsconfig.json
  2. write the tsconfig.cypress.json to the local repo

In the interest of getting this fixed asap, I wonder if we want a PR that just adds the fix, not any changes to the behavior?

My concern with the change is the tsconfig.json we generate will contain hard coded paths, which include the user's username:

{
  "extends": "/Users/lachlanmiller/code/dump/angular-app/tsconfig.app.json",
  "compilerOptions": {
    "outDir": "/Users/lachlanmiller/code/dump/angular-app/out-tsc/cy",
    "allowSyntheticDefaultImports": true,
    "skipLibCheck": true,
    "types": [
      "cypress"
    ]
  },
  "include": [
    "/Users/lachlanmiller/code/dump/angular-app/**/*.cy.ts",
    "/Users/lachlanmiller/code/dump/angular-app/cypress/support/component.ts"
  ]

Which means the committed files will only work on the original developer's machine. This is a blocker.

Happy to work towards having a local file, but we will need to fix this issue. In the meantime, how about a separate patch that addresses the core issue, and figure out the path issue separately (unless you know a fix right now).

Good call out, I changed the generated tsconfig.cypress.json file to use relative paths in commit cf58585

@jordanpowell88 jordanpowell88 requested review from lmiller1990 and a team June 26, 2023 19:47
@jordanpowell88 jordanpowell88 self-assigned this Jun 26, 2023
Copy link
Contributor

@barbados-clemens barbados-clemens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is 1 issue with the setup for nx workspaces which is an invalided path in the generated tsconfig, that would require some change to land the PR behavior as is without breaking existing Nx workspaces. though I personally think the behavior could be tweaked some to work better with existing Nx workspaces.

Talking with @jordanpowell88 I understand why the change is warranted, but is there a way to instead use the tsconfig in the cypress directory if it is present in the projectRoot or a way to tell Cypress Angular CT to generate/use a specific file instead of just defaulting to <projectRoot>/tsconfig.cypress.json

Couple of reasons for this.

  1. this adds another tsconfig in the project root for nx workspaces which can already be a lot, ideally we can tell cypress to generate in a sub directory 'cypress' folder to help keep things tidy. not a cypress specific issue but something that would be nice to help prevent config overload on our (Nx) side.
  2. Cypress already recommends using a tsconfig.json in the cypress directory so it would be advantageous to keep it there for consistency (if directory is present in the projectRoot) IMO.

thanks for giving me the heads up and letting me check out the changes before hand. helps a lot!

Comment on lines 128 to 125
includePaths.push(toPosix(cypressConfig.supportFile))
const supportPath = toPosix(path.relative(workspaceRoot, cypressConfig.supportFile))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This path being generated in an nx workspace creates a wrong path.

in an nx workspace we setup cypress in a sub directory of the project root.
i.e.

./ (git root)
└──
   apps/
   └── angular-app/
      ├── cypress/
      │  └── tsconfig.json
      ├── src/
      ├── cypress.config.ts
      ├── project.json
      ├── tsconfig.app.json
      ├── tsconfig.cypress.json
      ├── tsconfig.editor.json
      ├── tsconfig.json
      └── tsconfig.spec.json

the current impl, creates this tsconfig

{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "outDir": "./out-tsc/cy",
    "allowSyntheticDefaultImports": true,
    "skipLibCheck": true,
    "types": ["cypress"]
  },
  "include": [
    "src/**/*.cy.ts",
    "src/**/*.cy.js",
    "apps/angular-app/cypress/support/component.ts"
  ]
}

which is invalid causing type errors when building form the missing component.ts file

The value would need to be
"cypress/support/component.ts"

I believe in angular cli workspace the workspaceRoot and projectRoot are the same values, but in nx they are not. So maybe using projectRoot here instead.

@lmiller1990
Copy link
Contributor

There is 1 issue with the setup for nx workspaces which is an invalided path in the generated tsconfig, that would require some change to land the PR behavior as is without breaking existing Nx workspaces. though I personally think the behavior could be tweaked some to work better with existing Nx workspaces.

Talking with @jordanpowell88 I understand why the change is warranted, but is there a way to instead use the tsconfig in the cypress directory if it is present in the projectRoot or a way to tell Cypress Angular CT to generate/use a specific file instead of just defaulting to <projectRoot>/tsconfig.cypress.json

Couple of reasons for this.

  1. this adds another tsconfig in the project root for nx workspaces which can already be a lot, ideally we can tell cypress to generate in a sub directory 'cypress' folder to help keep things tidy. not a cypress specific issue but something that would be nice to help prevent config overload on our (Nx) side.
  2. Cypress already recommends using a tsconfig.json in the cypress directory so it would be advantageous to keep it there for consistency (if directory is present in the projectRoot) IMO.

thanks for giving me the heads up and letting me check out the changes before hand. helps a lot!

I agree having one less tsconfig.json to deal with is better. This is why we created the /tmp one in the first, so users do not need to commit it.

To loop back to the actual problem

The issue is because we are using a temp file for the tsconfig any types that get added to the extended tsconfig types array aren't able to resolve. Currently we manually add the Cypress types to the includes array.

I'm actually a bit confused now - why does changing the location of the tsconfig.json from /tmp to the project root actually fix the issue? Is there a less heavy handed solution that would avoid having to create a new tsconfig.json file? It seems like this will create issues for Nx people - also, once we write a new file, we can't easily walk back on this decision, since people will have already committed the file to source control. cc @jordanpowell88.

@barbados-clemens it would be great if we could use the cypress/tsconfig.json. One concern would be - any conflict there relating to End to End testing? I don't expect so, but I could be missing something. This does seem more ideal - keep cypress related things in the cypress directory (or the Nx project, for Nx users).

@jordanpowell88
Copy link
Collaborator Author

jordanpowell88 commented Jun 28, 2023

We should be able to resolve it by keeping it temporary and adding typeRoots that point to the root dir. The issue is the extended tsconfig types aren't resolved since it's in a temp dir.

The other option is what this PR introduces which is the ability for others to just manage the tsconfig themselves. It's a trade off each case.

@lmiller1990
Copy link
Contributor

We should be able to resolve it by keeping it temporary and adding typeRoots that point to the root dir.

To the /tmp/tsconfig.json right?

The issue is the extended tsconfig types aren't resolved since it's in a temp dir.

Is this intended behavior? Just to clarify, a minimal reproduction would be:

{ 
  "extends": "/tmp/tsconfig.json
}

^^^^^^^^ will not resolve typeRoots - why is it different for a file in a temporary directory?

(Could be misunderstanding your comment)

@lmiller1990
Copy link
Contributor

We should be able to resolve it by keeping it temporary and adding typeRoots that point to the root dir.

To the /tmp/tsconfig.json right?

The issue is the extended tsconfig types aren't resolved since it's in a temp dir.

Is this intended behavior? Just to clarify, a minimal reproduction would be:

{ 
  "extends": "/tmp/tsconfig.json
}

^^^^^^^^ will not resolve typeRoots - why is it different for a file in a temporary directory?

(Could be misunderstanding your comment)

Any thoughts on this @jordanpowell88 ?

@jordanpowell88 jordanpowell88 changed the title feat(webpack-dev-server): generate a local tsconfig file for Angular feat(webpack-dev-server): add typeRoots to generated tsconfig for angular Jul 10, 2023
@jordanpowell88 jordanpowell88 changed the title feat(webpack-dev-server): add typeRoots to generated tsconfig for angular fix(webpack-dev-server): add typeRoots to generated tsconfig for angular Jul 17, 2023
npm/webpack-dev-server/src/helpers/angularHandler.ts Outdated Show resolved Hide resolved
},
include: includePaths,
})
}, null, 2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this was just to get pretty formatting? Still needed since this is dumping to a temp dir?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed, but makes it "pretty"er 🤣

@jordanpowell88 jordanpowell88 merged commit 376795f into develop Jul 18, 2023
105 of 111 checks passed
@jordanpowell88 jordanpowell88 deleted the jordanpowell88/angular-tsconfig branch July 18, 2023 01:01
ryanthemanuel pushed a commit that referenced this pull request Jul 19, 2023
Co-authored-by: Stokes Player <stokes@cypress.io>
Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>
Co-authored-by: Jordan <jordan@jpdesigning.com>
Co-authored-by: semantic-release-bot <semantic-release-bot@martynus.net>
Co-authored-by: Matt Henkes <mjhenkes@gmail.com>
Co-authored-by: Chris Breiding <chrisbreiding@gmail.com>
fix: Fix web worker creation within spec frame (#27313)
fix: Handle privileged commands arg mutation (#27282)
fix(webpack-dev-server): add typeRoots to generated tsconfig for angular (#27117)
fix memory import (#27324)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Angular Component Testing cannot find type definition files
5 participants