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

feat(webpack): create build log for remotes to help debug errors #22539

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

jaysoo
Copy link
Member

@jaysoo jaysoo commented Mar 27, 2024

This PR adds a log file for users to inspect whenever the remotes fail to build.

Users will see an error like this when serving host fails at the build step:

> nx run shell:serve:development


 NX  Starting module federation dev-server for shell with 2 remotes


 NX  Building 2 static remotes...

/private/tmp/acme3/node_modules/@nx/react/src/executors/module-federation-dev-server/module-federation-dev-server.impl.js:121
                throw new Error(`Remote failed to start. A complete log can be found in: ${remoteBuildLogFile}`);
                ^

Error: Remote failed to start. A complete log can be found in: /private/tmp/acme3/.nx/cache/2024-03-27T15_48_01_673Z-build.log
    at ChildProcess.<anonymous> (/private/tmp/acme3/node_modules/@nx/react/src/executors/module-federation-dev-server/module-federation-dev-server.impl.js:121:23)
    at ChildProcess.emit (node:events:518:28)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:294:12)

Node.js v20.11.1

Current Behavior

Errors are swallowed since run-many is logging to stdout.

Expected Behavior

Errors should be surfaced, in this case, via a build log file.

Related Issue(s)

Fixes #

@jaysoo jaysoo requested review from a team as code owners March 27, 2024 16:16
Copy link

vercel bot commented Mar 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview Mar 27, 2024 6:19pm

@jaysoo jaysoo self-assigned this Mar 27, 2024
Copy link
Contributor

@Coly010 Coly010 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 duplicated just, but LGTM

Comment on lines +5 to +7
import { fork } from 'node:child_process';
import { join } from 'node:path';
import { createWriteStream } from 'node:fs';
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we run into issues with this style of import for users on older versions of Node?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's supported since Node 16, and we already use it in a few places.

Comment on lines +22 to +24
import { fork } from 'node:child_process';
import { basename, dirname, join } from 'node:path';
import { createWriteStream, cpSync } from 'node:fs';
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@jaysoo
Copy link
Member Author

jaysoo commented Mar 27, 2024

Merging this sine the failure is related to tailwindcss bug with 3.4.2, nothing to do with Nx. tailwindlabs/tailwindcss#13384

The failing e2e-angular-extensions tests will pass once tailwindcss releases a patch.

@jaysoo jaysoo enabled auto-merge (squash) March 27, 2024 19:12
@jaysoo jaysoo disabled auto-merge March 27, 2024 19:12
@jaysoo jaysoo merged commit 5381742 into master Mar 27, 2024
4 of 6 checks passed
@jaysoo jaysoo deleted the mf_build_log branch March 27, 2024 19:12
Copy link

github-actions bot commented Apr 2, 2024

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 2, 2024
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.

2 participants