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: CSS import in CT Support file is not working #24117

Merged
merged 19 commits into from
Oct 7, 2022
Merged

Conversation

amehta265
Copy link
Contributor

User facing changelog

Webpack >=4 adds a new feature: giving the user the ability to add sideEffects to their package.json files. If a user adds the CT Support file as a part of their side effect, all global application styles that may have been imported in the support file (e.g. import ./example.css) are not applied.

  • The change implemented ensures that all css files are flagged as containing side Effects and thus are not tree-shaken. This will ensure that there will be no dead-code removal of css files by Webpack and so all styles imported to the CT Support file will be enforced in the user's component irrespective of the sideEffects set by the user.

Additional details

  • Why the problem exists: Webpack 4 introduced several optimizations including dead code removal through tree shaking. Webpack follows all your import and export statements keeping track of the libraries and functions you use and don’t use. So, when it sees something is being imported but not being used, it considers that as ‘dead-code’ and will tree-shake it. But dead code is not always clear though. In our case, the CT support file imports *.css files but never utilizes them. Webpack believes this is dead code and therefore removes them. But this is inherently wrong. Some imports, simply by being included in the bundle have an important impact on the application e.g. global stylesheets and global configurations (in our case *.css and *.command.js are examples of this).
  • Other considered solutions:
  1. Scaffolding the user's package.json file and directly editing the sideEffects flag from there. This could be messy and could lead to a lot of architecture changes with how we interact and consume the package.json
  2. Setting sideEffects: true globally in the module.rules. This tells Webpack that all files contain sideEffects preventing any file from being tree shaken increasing bundle size and slowing down the application.
  3. Add optimization.sideEffects: false. This means that any sideEffects flag set in the package.json will be ignored. Issue is that if the user needs some files to be protected from tree shaking while running cypress, setting this to false could lead to a situation where the user’s sideEffects flag in their package.json is ignored and therefore their intended use case is not met.
  • Implemented solution: Set sideEffects: true for css modules through module.rules. In our case we want all css files to have a flag of sideEffects: true. This solution is better than setting global sideEffects: true as only css files will be prevented from tree shaking as compared to all the files in the latter option.

Steps to test

  1. Change directory to webpack-dev-server
  2. Run yarn cypress:open
  3. Run the react.cy.ts testing file. All the projects tested within this file have been configured to include sideEffects in their respective package.json. I have included an assertion to ensure that the background color is applied to the component after the global css files are imported into the CT Support file.

How has the user experience changed?

Before:
Screen Shot 2022-10-04 at 12 22 07 PM

After:
Screen Shot 2022-10-04 at 12 23 02 PM

PR Tasks

  • 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
Copy link
Contributor

cypress-bot bot commented Oct 4, 2022

Thanks for taking the time to open a PR!

@amehta265 amehta265 requested a review from a team October 4, 2022 16:45
@cypress
Copy link

cypress bot commented Oct 4, 2022



Test summary

41787 0 3397 0Flakiness 4


Run details

Project cypress
Status Passed
Commit 22c7bb9
Started Oct 7, 2022 2:53 PM
Ended Oct 7, 2022 3:12 PM
Duration 18:50 💡
OS Linux Debian - 11.4
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

project-setup.cy.ts Flakiness
1 ... > shows the configuration setup page when selecting e2e tests
specs_list_latest_runs.cy.ts Flakiness
1 App/Cloud Integration - Latest runs and Average duration > when no runs are recorded > shows placeholders for all visible specs
angular.cy.ts Flakiness
1 Working with angular-15 > should mount a passing test
cypress-in-cypress-component.cy.ts Flakiness
1 Cypress In Cypress CT > default config > moves away from runner and back, disconnects websocket and reconnects it correctly

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

@astone123 astone123 requested a review from a team October 5, 2022 14:16
@astone123 astone123 requested a review from a team October 5, 2022 16:16
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.

I left a minor comment. Looks good to me! Nice work on this 🎉 definitely want to get some input from others about this solution just to make sure we're doing the right thing here

@astone123 astone123 requested a review from a team October 6, 2022 19:37
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.

Just a few comments. I realized that Cypress + sideEffects is not ideal. Makes me almost want to disable them entirely (maybe with this). The reason is that it doesn't just affect css files but any code that's imported like import 'xxx'. The import './commands.js' would be tree-shaken away as well, so it's not just css. Still, I think this is an improvement.

@astone123
Copy link
Contributor

@ZachJW34 I didn't see that optimization.sideEffects configuration value. That would be cool if we could always just disable side effects completely 💡

…nto issue-23025

merging with remote branch
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 and simple

@ZachJW34 ZachJW34 merged commit 5af6b27 into develop Oct 7, 2022
@ZachJW34 ZachJW34 deleted the issue-23025 branch October 7, 2022 16:24
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 11, 2022

Released in 10.10.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v10.10.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Oct 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS import in component is not respected
5 participants