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

chore: correctly resolve url sass bundle in Angular CT #25191

Merged
merged 4 commits into from Dec 16, 2022

Conversation

lmiller1990
Copy link
Contributor

@lmiller1990 lmiller1990 commented Dec 16, 2022

User facing changelog

Correctly resolve url in scss (eg background-image: url(....)).

Additional details

Angular uses webpack internally. It maps urls in sass via resolve-url-loader. We need to provide the correct root url - Angular assumes / by default, but we serve the assets on the dev server on __cypress/src.

Known edge case: url in .css files doesn't map properly. .scss and .sass do, though. We could hack it to force .css use sass-loader for css, but that's not really ideal - it's not what their app would be using in prod. In general, to work around it for now, we can just recommend using .scss - it's in Angular out of the box, and it's a superset of CSS .

This is also an issue for other frameworks (like React) using url in scss. Basically, anything using url in css and scss may exhibit this bug. The problem is more fundamental - I think we are not routing some asset requests properly for CT. I tried tweaking the routing in #25120 but it's too involved right now, and a bit more risky to touch routing, since that impacts the entire of Cypress. This routing issue (at least, I think it's routing) bleeds into other things, too, like this bug: mswjs/msw#744.

I will spend more time thinking about this in the future, but for now I'd like to get Angular working out of the box. Showing images is a pretty basic thing that should work. This PR is the most simple and low risk way to do so.

Steps to test

Grab the repro in the issue from the original user and verify the problem where the images don't show no longer is a problem.

How has the user experience changed?

url for images correctly resolves.

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)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 16, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Dec 16, 2022



Test summary

26138 0 1377 0Flakiness 26


Run details

Project cypress
Status Passed
Commit 1afe607
Started Dec 16, 2022 4:04 PM
Ended Dec 16, 2022 4:22 PM
Duration 18:06 💡
OS Linux Debian -
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

migration.cy.ts Flakiness
1 global mode > migrates 2 projects in global mode
commands/net_stubbing.cy.ts Flakiness
1 ... > with `resourceType` > can match a proxied image request by resourceType
2 ... > stops waiting when an xhr request is canceled
3 ... > multibyte utf8 > 12345678901234567890123Ф
e2e/origin/cookie_behavior.cy.ts 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
This comment includes only the first 5 flaky tests. See all 26 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 marked this pull request as ready for review December 16, 2022 14:00
@lmiller1990 lmiller1990 changed the title chore: correcty resolve url sass bundle in Angular CT chore: correctly resolve url sass bundle in Angular CT Dec 16, 2022
@lmiller1990 lmiller1990 requested review from jordanpowell88 and a team December 16, 2022 14:02
Copy link
Contributor

@rockindahizzy rockindahizzy left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@ZachJW34 ZachJW34 left a comment

Choose a reason for hiding this comment

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

💯

@ZachJW34 ZachJW34 merged commit 331c1dc into develop Dec 16, 2022
@ZachJW34 ZachJW34 deleted the lmiller/issue-24272-angular-resolve-url branch December 16, 2022 16:32
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.

Error with assets loading from scss in Angular
3 participants