fix(webpack-preprocessor): hanging issues with webpack 5

[lukeapage]

• Closes Cypress extreme slow initial test with Webpack 5 + Typescript #15447
• Closes webpack-preprocessor: Add webpack 5 as peerDependency #16947

User facing changelog

• Cypress no longer hangs intermittently when using webpack 5

Additional details

#15447 (comment)

I'm not sure what the best fix is - this approach at least means that you will not get promises hanging. I don't know if it could in some circumstances cause an old version of the file to be served.

I'm unsure on whether tests are required - I would at least like to hear an opinion on the approach first.

PR Tasks

• Have tests been added/updated?

[cypress-bot]

Thanks for taking the time to open a PR!

• Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
• Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

[CLAassistant]

All committers have signed the CLA.

[chrisbreiding]

Thanks for you work on this. Sorry for the delay in reviewing it.

I think your approach is probably the way to go. I don't think there should be any risk of getting an old version of the file since all the promises will resolve with the same path and since they resolve after the latest compile has finished, the latest version of the file will be served to any client that has the older or newer promise.

We definitely need a few tests around this. One for the fix itself, one that verifies the deferred array gets cleared when compile succeeds, and one that verifies the deferred array gets cleared when compile fails.

[jennifer-shehane]

@lukeapage Will you have time to add a test and address the questions from the PR review? Thanks for the work again.

[lukeapage]

Been a bit busy recently and we have a workaround so this hasn’t been a top priority. I will try to make time soon.

[melibe23]

should I go back to webpack 4 or you are about to merge this fix? thank you!

[ghost]

How does this interact with #16493 by @lmiller1990 (which claims "can use with webpack v5")?
Are they fixing unrelated problems, or are they fixing the same problem in different ways?

[lukeapage]

Different things. This fixes a intermittent issue that occurs with webpack 5.

[lmiller1990]

I am a bit curious around the change relating to return Promise.delay(11). It seems like a race condition - someone on a slower machine may need a larger (or smaller) number.

Either way, I can pick this up. How did you recreated and test this issue @lukeapage? I haven't encountered it in my projects that use webpack 5 (those are pretty trivial projects, though). Did you use the repository provided in this comment?

[initplatform]

I think I have the same issue. It hangs forever when I launch a test, but if I click on the "Run All Tests" button it starts working.

I've upgraded and downgraded to webpack 5 multiple times... don't really want to downgrade again :(

[lmiller1990]

Hey @initplatform, I would really like to iron out the webpack 5 problems. Can you share a repo that reproduces this issue? It's not entirely clear if this will fix the issue for everyone.

Alternatively, does this PR fix the problem for you (if you cannot share your code base). Could you try it out locally? You could just modify the code in node_modules from this PR. That would at least give me a bit more confidence before merging this. Happy to put some time into getting this one merged up, just want to make sure we cover our bases and are actually fixing the problem at the core.

If there is a reliable way to reproduce this issue in a minimal repo, that'd be great, too.

[xgehl]

Hey @lmiller1990
I have the same issue and same workaround as @initplatform (except I had to spam that refresh button), but went back to webpack 4 as I was unusable....

I can't share the code, and not sure if I can reproduce it easily in a fresh project, but I gonna try the modifications locally this week-end,

[initplatform]

@lmiller1990 Unfortunately I am unable to post the project, and not sure entirely how I would recreate it...

However, I did take your changes and try them out and they do work!

I didn't want to piece together the transpiled typescript, so I checked out cypress and built it with your changes.

@xgehl, If you want to try out the changes, just put the file from this gist into node_modules/@cypress/webpack-preprocessor/dist/index.js

https://gist.github.com/initplatform/e2353db94c1270b89eaa07740a4b44c0

[xgehl]

@lmiller1990 I've tried the changes (thanks @initplatform for the file), and I can confirm it's working !

[lmiller1990]

I noticed an exception getting raised in the tests. I added this defensive check. Specifically it was this test that was raising an exception. Not sure if this fix is correct - may need to dive deeper into this part of the code base to really grok what's going on.

I merged in develop, and added some unit tests as requested by @chrisbreiding.

• 6f3a5fb
• 7753121

I'd appreciate another set of eyes @chrisbreiding - do you see any more thorough way to test this change, or any edge cases?

[lukeapage]

Thanks for helping to finish this @lmiller1990

[lmiller1990]

Ok @chrisbreiding taking a different approach, since I don't see a good way to easily update the preprocessor to run on webpack 5, but everything else run with webpack 5.

Have a look at this file. It will update package.json to webpack 5 before the tests, run yarn install, then revert to webpack 4 after. We then get coverage for both versions (webpack 5 via e2e/unit tests, webpack 4 via our other packages running with webpack 4 and using the processor).

It works fine, but the trade off is it adding some technical debt (this script). Mid term, I'd like to upgrade all of Cypress to use webpack 5.

[chrisbreiding]

Looks good to me. Nice solution.

I agree we should revisit in the future to get the rest of the codebase on webpack 5 and add proper testing in this package for webpack 4, since we still support it.

[chrisbreiding]

server-integration-tests is failing consistently and doesn't appear to be failing in develop, so I worry this change is responsible somehow.

[lmiller1990]

The changes here somehow lead to this error getting thrown in the plugins process. It now manifests in the launcher - it should be in the runner. That's what the server-integration-tests failure is telling us.

It's because we are rejecting everything here. I am not sure how best to solve this... 🤔 . On develop we end up calling this part of the code which does not happen after this change.

This is where things change. In develop we correctly reject the promise and catch it; in this branch, we don't.

[lmiller1990]

Ok... found a fix (kind of). Bit of a hack, but we resolve every bundle for a successful compilation. For an error, we only want one rejection, so we just reject the last bundle.

348b6c4

Tests passing, did a bit of testing by hand too, and seems to be working. This doesn't feel ideal, but I don't have another solution right now.

Let me know what you think @chrisbreiding.

[chrisbreiding]

I agree it's not ideal, but I can't think of another solution either.

[lukeapage]

@chrisbreiding @jennifer-shehane the bug this resolves has just been closed with a note that it’s released but as this is in a different package, it is actually not released yet. The issue is locked so I cannot bring it up there.

[lmiller1990]

Looks like this was released 6 hours ago (8 hours after your post) via semantic release @lukeapage.