Skip to content

fix: use srcRoot for angular build context #25090

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 16, 2022
Merged

fix: use srcRoot for angular build context #25090

merged 4 commits into from
Dec 16, 2022

Conversation

ZachJW34
Copy link
Contributor

@ZachJW34 ZachJW34 commented Dec 9, 2022

User facing changelog

Fix issue with Angular CT where custom srcRoot was not being respected.

Additional details

We were passing in root rather than srcRoot, causing some path issues.

I wasn't able to reproduce the exact issue the user is having. I'm building the binaries for this PR and will share to the user.

Steps to test

  1. npm i @angular/cli -g (you'll need a global version >=13 and <= 15)
  2. ng new Choose default options
  3. Rename src folder to ui
  4. Rename all instances of src in tsconfig.app.json and angular.json to ui
  5. Verify app still serves with ng serve
  6. npm i -D cypress && npx cypress open
  7. Walk through component setup, should error when loading config
  8. Kill Cypress, check out this branch and run yarn workspace @cypress/webpack-dev-server build
  9. yarn cypress:open --project <path-to-angular-project>
  10. Click on CT, no error when loading config.

How has the user experience changed?

Screen.Recording.2022-12-09.at.9.59.43.AM.mov

PR Tasks

  • Have tests been added/updated?
  • 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
Copy link
Contributor

cypress-bot bot commented Dec 9, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Dec 9, 2022 •
edited
Loading


Test summary

10172 • 0 • 254 • 0 • Flakiness 10

Run details
Project cypress
Status Passed
Commit 44299f3
Started Dec 16, 2022 2:12 PM
Ended Dec 16, 2022 2:28 PM
Duration 15:51 💡
OS Linux Debian -
Browser Multiple

View run in Cypress Dashboard ➡️

Flakiness

cypress/e2e/e2e/origin/cookie_behavior.cy.ts 5 Flakiness
1 ... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
2 ... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
3 ... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
4 ... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
5 ... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
This comment includes only the first 5 flaky tests. See all 10 flaky tests in the Cypress Dashboard.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

lmiller1990
lmiller1990 approved these changes Dec 11, 2022
View reviewed changes
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.

Some QoL comments, no obvious problems - sure would be nice to see if this works for the user.

It's also possible

system-tests/projects/angular-custom-root/ui/main.ts
@@ -0,0 +1,8 @@
import { platformBrowserDynamic } from '@angular/platform-browser-dynamic'
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need a basic Angular project in project-fixtures at some point, so we iterate over it like we do for React in system tests, testing against each supported major 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have one, but this project felt different enough that I thought I'd make a new one. If I was to reuse the angular project-fixture, I'd have had to overwrite almost every file (every config file + the src code is in a different directory)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @ZachJW34 that this is worth a different project since the source root is pretty fundamental part of the app.

npm/webpack-dev-server/src/helpers/angularHandler.ts
@@ -217,7 +217,7 @@ function createFakeContext (projectRoot: string, defaultProjectConfig: Cypress.A
getProjectMetadata: () => {
return {
root: defaultProjectConfig.root,
sourceRoot: defaultProjectConfig.root,
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this API change between majors? Should we check defaultProjectConfig.root ?? defaultProjectConfig.sourceRoot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it didn't change, I believe it was just a mistake on our part.

astone123
astone123 approved these changes Dec 12, 2022
View reviewed changes
Copy link
Contributor

@astone123 astone123 left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻 would be nice to confirm that this solves the user's issue specifically

jordanpowell88
jordanpowell88 approved these changes Dec 12, 2022
View reviewed changes
Copy link
Contributor

@jordanpowell88 jordanpowell88 left a comment

Choose a reason for hiding this comment

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

Great job with this. This was certainly an oversight on our end

@ZachJW34
Copy link
Contributor Author

Going to hold off on merging for a day or so, hoping the original poster of the issue has a chance to try out the binary

@ZachJW34
Copy link
Contributor Author

Going to go ahead and get this merged.

@ZachJW34 ZachJW34 merged commit 7c36118 into develop Dec 16, 2022
@ZachJW34 ZachJW34 deleted the zachw/angular-ct-source-root branch December 16, 2022 15:13
@cypress-bot cypress-bot bot mentioned this pull request Dec 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordanpowell88 jordanpowell88 jordanpowell88 approved these changes

@astone123 astone123 astone123 approved these changes

@lmiller1990 lmiller1990 lmiller1990 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to change the source root
4 participants
@ZachJW34 @jordanpowell88 @astone123 @lmiller1990