chore: update to webpack v5 #27438
chore: update to webpack v5 #27438
Conversation
AtofStryker
changed the title
7 flaky tests on run #49630 ↗︎
Details:
|
|||||||||||||||||||||||||||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| ... > shows No Specs page with specPattern from config |
Output
Screenshots
Video
|
|
cypress\e2e\create-from-component.cy.ts • 1 flaky test • app-e2e
| Test | Artifacts | |
|---|---|---|
| ... > Shows create from component card for React projects |
Output
Screenshots
Video
|
|
cypress\e2e\runs.cy.ts • 1 flaky test • app-e2e
| Test | Artifacts | |
|---|---|---|
| App: Runs > no internet connection > shows correct message on create org modal |
Output
Screenshots
Video
|
|
cypress\e2e\cypress-in-cypress.cy.ts • 1 flaky test • app-e2e
| Test | Artifacts | |
|---|---|---|
| Cypress in Cypress > scales the AUT correctly in component |
Output
Screenshots
Video
|
|
cypress\e2e\specs_list_latest_runs.cy.ts • 1 flaky test • app-e2e
| Test | Artifacts | |
|---|---|---|
| App/Cloud Integration - Latest runs and Average duration > when no runs are recorded > shows placeholders for all visible specs |
Output
Screenshots
Video
|
|
The first 5 flaky specs are shown, see all 7 specs in Cypress Cloud.
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.
|
This should also add support for many historic issue that had "fixes", now those will work out of the box. Example issues:
And any others relating to wepback compilation errors and newer JS syntax. |
There was a problem hiding this comment.
Left some comments / questions, looks good!
Unresolved question: how to release this? I think this will be a Cy 13 breaking change. Migration path isn't clear, though - if there's some way to modify the webpack config without installing @cypress/webpack-preprocessor, this is going to be hard.
I suspect this IS NOT a problem. The reason is the docs say:
Note that installing @cypress/webpack-preprocessor is also required. This allows you to update its version separately from this wrapper.
npm install --save-dev @cypress/webpack-batteries-included-preprocessor @cypress/webpack-preprocessor
So anyone who wanted to this is on the OLD version of preprocessor. Since we will make this breaking, it'll bump the major. If they upgrade, they'll need to make changes. We might need some notes on how to do that, but they could just refer to the webpack 4-5 migration guide.
| * in order to guarantee there is a version of the dependency accessible by the cypress CLI, either in the cypress directory | ||
| * or the root of their project. Currently, these two dependencies are 'buffer' and 'process' | ||
| */ | ||
| describe('dependencies', () => { |
There was a problem hiding this comment.
I have some questions about this. If webpack 4 polyfilled all core Node.js libs out of the box, does that mean you could (and can)
- install cypress 12
- write a spec that imports
fs - have it run successfully (eg,
fswon't error out)
I haven't tried this yet, but worth considering. I know we don't do this in our code base, but I'm sure some code bases rely on those polyfills (especially crypto).
I wonder if we need to polyfill ALL the core modules to maintain backwards compatibility.
There was a problem hiding this comment.
I think it depends. fs is probably a bad example here since we intentionally leave it empty in the webpack-batteries-included-preprocessor and don't expect that built in to be available. But for something like crypto for your example, you could do all those steps and it will not error out since we install the polyfill. I think you talked about it in another spot in this PR, but this comment explains it for anyone else reading
|
|
||
| expect(dependencies.buffer).to.be.ok | ||
|
|
||
| const buffer = require('buffer') |
There was a problem hiding this comment.
Should we assert that
require('node:buffer') !== require('buffer')That would verify the Buffer you get is not the core Node.js one. You can use require('node:<module>') to get the core one.
There was a problem hiding this comment.
that might be a good idea, considering we really want to just make sure the buffer is installed in the package through the package.json and its accessible
| stream: require.resolve('stream-browserify'), | ||
| string_decoder: require.resolve('string_decoder/'), | ||
| sys: require.resolve('util/'), | ||
| timers: require.resolve('timers-browserify'), |
There was a problem hiding this comment.
Oh, maybe we do polyfill them all. My previous comment may be irrelevant.
Big breaking change here - who knows if these implementations match whatever webpack 4 was doing.
Tracking down the right polyfill here must have been complex.
There was a problem hiding this comment.
Oh, maybe we do polyfill them all. My previous comment may be irrelevant.
Big breaking change here - who knows if these implementations match whatever webpack 4 was doing.
This for sure breaks the webpack-batteries-included-preprocessor . In this case, we polyfill most of them and are fairly explicit about the ones we don't in the config. We also have a few tests in place since just about everything goes through the preprocessor. The tests that we do check for the polyfills are this system test in plugins_spec.js which uses this spec and checks for just about every polyfill in this gist except for the ones starting with _, in particular the readable-stream dependencies. There is also this unit test that pretty much tests the same thing. At least for the built ins we are want to convey we support we should be covered.
Tracking down the right polyfill here must have been complex.
this gist for sure helped to figure out what was going on
| const nextBundle = utils.createDeferred<string>() | ||
| /** | ||
| * Webpack 5 fix: | ||
| * If the bundle is the initial bundle, do not create the deferred promise |
There was a problem hiding this comment.
Nice job figuring this out 💯
This is the big thing to figure out here. Unless there is a way to ship new major versions of the |
|
I think it's possible to ship this early but bundle the older version in the binary. If Cy 13 is going out this month, it's not worth it, if we expect delays, we should look into it, though. |
|
@lmiller1990 I updated the PR description to describe how we are going to release this. We will first merge this in as a |
|
@lmiller1990 linked #27469 in the PR description in draft. I think this is ready for a re review 😄 |
AtofStryker
force-pushed
the
chore/update_to_webpack5_minimum_lift
branch
from
6f196e1 to
4934d73
Compare
| @@ -7,6 +7,10 @@ _Released 08/15/2023 (PENDING)_ | |||
|
|
|||
| - Fixed an issue where having `cypress.config` in a nested directory would cause problems with locating the `component-index.html` file when using component testing. Fixes [#26400](https://github.com/cypress-io/cypress/issues/26400). | |||
|
|
|||
| **Dependency Updates:** | |||
|
|
|||
| - Upgraded [`webpack`](https://www.npmjs.com/package/webpack) from `v4` to `v5`. This means that we are now bundling your `e2e` tests with webpack 5. We don't anticipate this causing any noticeable changes. However, if you'd like to keep bundling your `e2e` tests with wepback 4 you can use the same process as before by pinning [@cypress/webpack-batteries-included-preprocessor](https://www.npmjs.com/package/@cypress/webpack-batteries-included-preprocessor) to `v2.x.x` and hooking into the [file:preprocessor](https://docs.cypress.io/api/plugins/preprocessors-api#Usage) plugin event. This will restore the previous bundling process. Additionally, if you're using [@cypress/webpack-batteries-included-preprocessor](https://www.npmjs.com/package/@cypress/webpack-batteries-included-preprocessor) already, a new version has been published to support webpack `v5`. | |||
There was a problem hiding this comment.
Would they also need to install webpack@4 to keep things working the same?
There was a problem hiding this comment.
not in this case because v2 of webpack-batteries-included-preprocesor ships with 4. its not backwards compatible unlike the regular webpack-preprocessor
https://webpack.js.org/migrate/5/#make-sure-your-build-has-no-errors-or-warnings https://webpack.js.org/migrate/5/#make-sure-to-use-mode https://webpack.js.org/migrate/5/#update-outdated-options https://webpack.js.org/migrate/5/#test-webpack-5-compatibility app builds and runs locally. Time to test out in CI and see if buffer or process need to be polyfilled by the build
…reprocessor to be webpack 5 compliant
[email protected] which has a dependency on punycode node expected API. since punycode is now polyfilled for us implicitly via the punycode npm package, the API signatures are a bit different https://github.com/mathiasbynens/punycode.js/blob/main/punycode.js#L101 vs https://nodejs.org/api/punycode.html#punycodeucs2. The patch uses the punycode npm package expected API and is needed for source maps to work for cy.origin() dependencies for Cypress.require()
…lled when building the binary / installing prod dependencies
…t to avoid having to polyfill Buffer from webpack
…e yarn_v3.1.1_pnp_spec.ts system test. see webpack/enhanced-resolve#263 for more details
…ifferent webpack compiled terminal formatting in the snapshot between local and CI.
…ed-preprocessor correctly
…o impact due to the provide plugin
5 added by webpack-terser-plugin under the hood
…ed as a dep as its used in the config [run ci]
AtofStryker
force-pushed
the
chore/update_to_webpack5_minimum_lift
branch
2 times, most recently
from
5d45654 to
721d6a5
Compare
|
jobs are passing but for whatever reason the snyk workflows wont run. We are going to admin merge this branch is barring everything else in CI is passing. Passing snyk jobs from previous runs to show no vulnerabilities to build deps https://github.com/cypress-io/cypress/actions/runs/5730373496 & https://github.com/cypress-io/cypress/actions/runs/5730373491 |
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
What is in this PR
This PR updates the
cypressmonorepo to usewebpack@5fromwebpack@4doing the bare minimum updating to get webpack 5 working. This handles the Test webpack 5 compatibility and Upgrade webpack to 5 steps in the migration guideThe main goal of updating to webpack 5 is to get with the recent dependency updates, as well as unblock the electron v25 upgrade. Since newer versions of Electron run on Node 18, it causes issues with
--openssl-legacy-provider, which is fixed by this upgrade (see code removed in b973ca5 ). SeeMost the changes needed are well documented inside the code as to why we need them. This includes
whatwg-urland another toenhanced-resolvedue to polyfilled differences (see code). In addition to these patches and to make sure they are applied correctly, dev dependency patches are not carried over to the build directory (see 53e304f).webpack.base.config.tsinsideweb-configfor Cypress internally, as well as polyfilling correctly for thewebpack-batteries-included-preprocessorthrough resolve.fallback, ProvidePlugin, and DefinePluginpathpolyfill code (see f01f645)webpack-preprocessortime out (see 1fa6183). This promise reference seems to be preserved correctly inwebpack@4, which seems odd since it should not be as thewebpack@5handles this correctly.system-tests, mainly due to difference source-map paths and console outputsPerformance considerations
@mjhenkes and I looked into the telemetry data on the initial green build of the run. We both feel confident the performance currently is about the same and no major regressions are present.
What is NOT in this PR
Loader/Plugin updates are not in this PR as they are not needed to update to
webpack@5. However, we should update to the latest loaders to gain benefits of recent versions, which will be done in a follow up PRAdditionally, there are still build warnings that need to be fixed from the
webpack@5update. These will also be addressed in a follow up PR to prevent any breakages when deprecated behavior is removed.Optimizations to the build also need to be performed, which will be addressed in a separate PR. see clean up configuration in the migration guide.
Is this a breaking change to
cypress?This is NOT a breaking change to
cypress, however it does change the way cypress is bundled under the hood. This should not break end users. However, since polyfills have been removed inwebpack@5, we need to make sure that dependencies that are bundled via the ProvidePlugin are available dependencies in a user's project ifcypressis installed. Otherwise, bundling could fail. This means thatbufferandprocessneed to be installed with the CLI in the case a user does NOT havebufferorprocessas a fallback in their dependency tree. Other polyfills that are set through resolve.fallback are installed locally to the dependency (such aswebpack-batteries-included-preprocessor) and can be discovered at processing time without having to hoist/install dependencies in the users rootnode_modules(see 697f8bf).The other aspect that might be considered breaking is the subtle changes to the node built ins from
webpack@4towebpack@5. We know some of the APIs have changed slightly to the built ins. We have discussed this at length at Cypress, and will NOT be considering this a breaking change. The likelihood users are broken by built in changes is incredibly small, and in the case this is a problem, the user can either:webpack@4If users are using the
file:preprocessorhook insideplugins, users can still usewebpack@4still and will not be broken, even though cypress is bundled with webpack 5. The only stipulation to continuing usingwebpack@4is that users remain on Node 16 aswebpack@4has this issue that will not be backpatched from webpack. We recommend users upgrade to recent versions of webpack and node, as Node 16 will be EOL in Q3 of 2023Do we need to publish new major versions of
webpack-preprocessor,webpack-dev-server, andwebpack-batteries-included-preprocessor?The only package that will require a major version publish from this change will be
@cypress/webpack-batteries-included-preprocessoras it will be shipping withwebpack@5from now on. The breaking changes will be included in this PR, but will NOT be published in this PR as it is flagged as achoreand NOTBREAKING_CHANGE. TheBREAKING_CHANGEcommit will be handled in a subsequent PR (see #27469). The strategy for release of this PR will be:choreintodevelop. Nothing is releaseddevelopas aBREAKING_CHANGEto@cypress/webpack-batteries-included-preprocessor. This publishes the new version of the package to npm.Steps to test
Give it a run for yourself locally and make sure everything is good on your end, as well as downloading and testing the binary. We have a green circle build for CI testing to make sure everything is good there.
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?