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

refactor: use cp.fork to preserve stdout for packages/extension build #22926

Merged
merged 3 commits into from Jul 29, 2022

Conversation

ZachJW34
Copy link
Contributor

User facing changelog

na

Additional details

The extension package executes webpack inside of a gulp script. It was using a promisified exec script to invoke the run-webpack.js script and it wasn't piping any of the stdout. This PR uses fork to invoke webpack, making it easy to capture stdout with stdio: 'inherit'.

I ran into this when working through adding Angular CT support. Webpack 5 got hoisted and broke the webpack build but the error message was unclear. Now, if this script errors it will print the webpack logs.

Steps to test

Run yarn workspace @packages/extension build and you'll see the webpack logs. You can tweak the src files to throw a compilation error to verify that a non-zero exit code causes the gulp task to fail.

How has the user experience changed?

Before:
Screen Shot 2022-07-25 at 5 53 53 PM

After:
Screen Shot 2022-07-25 at 5 47 52 PM

PR Tasks

  • [na] 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?

@ZachJW34 ZachJW34 requested a review from a team as a code owner July 25, 2022 22:55
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jul 25, 2022

Thanks for taking the time to open a PR!

@lmiller1990
Copy link
Contributor

I ran into this exact same thing when updating to Vite 3, and could not figure out the error w/o stdout. Good fix.

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.

One comment, not a blocker, just offering an alternative.

@@ -15,7 +16,11 @@ const manifest = () => {
}

const background = (cb) => {
exec('node ../../scripts/run-webpack.js').then(() => cb()).catch(cb)
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought exec had a second arg where you could pass

exec('node ....', { stdout: ... }

But now I realize that is execa. Another option would be to grab the stdout from the callback:

exec('node ...', (error, stdout, stderr) => {
  // stuff
})

It might be more idiomatic to do something like this, and do a if (err) check?

Either way is fine with me, I'll leave the final decision to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the current implementation as it preserves the ansii highlighting, whereas console logging the stdout does not.

@cypress
Copy link

cypress bot commented Jul 25, 2022



Test summary

4938 0 59 0Flakiness 0


Run details

Project cypress
Status Passed
Commit ca3811b
Started Jul 29, 2022 5:56 PM
Ended Jul 29, 2022 6:09 PM
Duration 13:06 💡
OS Linux Debian - 11.3
Browser Electron 102

View run in 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

@chrisbreiding chrisbreiding removed their request for review July 26, 2022 00:03
@ZachJW34 ZachJW34 force-pushed the zachw/fix-extension-webpack-logging branch from 60329d7 to ca3811b Compare July 29, 2022 16:53
@ZachJW34 ZachJW34 merged commit 2fd495c into develop Jul 29, 2022
@ZachJW34 ZachJW34 deleted the zachw/fix-extension-webpack-logging branch July 29, 2022 19:03
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.

None yet

2 participants