Skip to content

fix(angular): optimize ng-packagr-lite watch mode file writing#31434

Merged
Coly010 merged 1 commit intomasterfrom
issue-31033-fix
Jun 5, 2025
Merged

fix(angular): optimize ng-packagr-lite watch mode file writing#31434
Coly010 merged 1 commit intomasterfrom
issue-31033-fix

Conversation

@Coly010
Copy link
Copy Markdown
Contributor

@Coly010 Coly010 commented Jun 3, 2025

Current Behavior

In ng-packagr-lite watch mode, all output files are written to disk on every incremental build regardless of whether their content actually changed. This causes downstream watchers (like Vite) to see all files as "changed" and trigger full rebuilds instead of incremental ones.

Expected Behavior

Only files with changed content should be written to disk during incremental builds in watch mode. This allows downstream watchers to properly detect which files actually changed and perform efficient incremental rebuilds.

Related Issue(s)

Fixes #31033

🤖 Generated with Claude Code

@Coly010 Coly010 requested a review from a team as a code owner June 3, 2025 10:43
@Coly010 Coly010 requested a review from leosvelperez June 3, 2025 10:43
@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 3, 2025

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

Name Status Preview Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview Jun 5, 2025 9:28am

@nx-cloud
Copy link
Copy Markdown
Contributor

nx-cloud Bot commented Jun 3, 2025

View your CI Pipeline Execution ↗ for commit fb8d31b.

Command Status Duration Result
nx affected --targets=lint,test,build,e2e,e2e-c... ✅ Succeeded 23m 4s View ↗
nx run-many -t check-imports check-commit check... ✅ Succeeded 16s View ↗
nx-cloud record -- nx format:check ✅ Succeeded 3s View ↗
nx-cloud record -- nx sync:check ✅ Succeeded 2s View ↗
nx documentation ✅ Succeeded 2m 33s View ↗

☁️ Nx Cloud last updated this comment at 2025-06-05 09:45:25 UTC

newContent: string
): Promise<boolean> {
try {
if (!existsSync(filePath)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's a potential race condition in this code. The existsSync() check followed by readFile() isn't thread-safe - the file could be deleted between these operations. Since the try-catch already handles file read failures, you can simplify by removing the existence check and directly attempting to read the file:

async function shouldWriteFile(filePath: string, newContent: string): Promise<boolean> {
  try {
    const existingContent = await readFile(filePath, 'utf-8');
    return existingContent !== newContent;
  } catch (error) {
    // If we can't read the existing file (including if it doesn't exist), write the new one
    return true;
  }
}

This approach is more robust and eliminates the race condition while maintaining the same functionality.

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@claude can you apply this suggestion

Copy link
Copy Markdown
Contributor

@claude claude Bot Jun 3, 2025

Choose a reason for hiding this comment

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

Claude finished @Coly010's task in 1m 22s —— View job


Applied Review Suggestion ✅

Successfully applied the suggested refactor to eliminate the race condition in the shouldWriteFile function.

Todo List:

  • Read the current file implementation
  • Apply the suggested refactor to remove race condition
  • Commit the changes to the PR branch

Changes applied:

  • Removed the existsSync import and check
  • Simplified the function to directly attempt reading the file
  • The try-catch now handles both file existence and read errors in one place

This change makes the code more robust by eliminating the potential race condition between the file existence check and the actual file read operation. The function now relies solely on the try-catch mechanism to handle all error cases, including file not found errors.

Commit: d6d3796


@Coly010 Coly010 closed this Jun 3, 2025
@Coly010 Coly010 reopened this Jun 3, 2025
@Coly010 Coly010 force-pushed the issue-31033-fix branch 2 times, most recently from 0111568 to 023d605 Compare June 3, 2025 15:23
@Coly010
Copy link
Copy Markdown
Contributor Author

Coly010 commented Jun 3, 2025

@claude can you resolve the file conflicts in this PR

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jun 3, 2025

Claude finished @Coly010's task in 1m 55s —— View job


I'll help you resolve the file conflicts in this PR.

Todo List:

  • Check current git status for merge conflicts
  • Examine any conflicted files
  • Resolve conflicts while preserving functionality
  • Run validation commands
  • Commit the resolution

✅ Merge Conflicts Resolved Successfully

The merge conflict in write-bundles.transform.ts has been resolved! Here's what was done:

Conflict Details:

  • Issue: Import statements conflicted between the PR branch and master
  • PR branch: Added readFile import (needed for the shouldWriteFile function)
  • Master branch: Added normalize import (needed for path handling)

Resolution:

  • Combined both import requirements into a single import statement
  • Preserved all functionality from both branches
  • Formatted the code with prettier to maintain consistency

Changes Made:

  1. 3d94715: Resolved merge conflicts by combining import statements
  2. b1b79ea: Formatted code with prettier after conflict resolution

The PR is now ready for review and should merge cleanly with the master branch. The functionality for optimizing ng-packagr-lite watch mode file writing has been preserved along with the latest changes from master.


@Coly010
Copy link
Copy Markdown
Contributor Author

Coly010 commented Jun 3, 2025

@claude the latest commit message does not follow the conventional commits format. It is missing a scope. Can you amend the commit message to start with chore(angular):

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jun 3, 2025

Claude finished @Coly010's task in 58s —— View job


I'll fix the commit message to follow the conventional commits format with the proper scope.

Todo List:

  • Check current commit history to identify the commit that needs amending
  • Amend the commit message to start with chore(angular):
  • Push the updated commit to the PR branch

✅ Commit Message Fixed Successfully

The latest commit message has been updated to follow the conventional commits format:

Before: chore: format code with prettier after merge conflict resolution
After: chore(angular): format code with prettier after merge conflict resolution

Changes Made:

  • Amended the commit message to include the (angular) scope as requested
  • Force pushed the updated commit to the PR branch

The commit now properly follows the conventional commits format with the appropriate scope for Angular-related changes.

In ng-packagr-lite watch mode, all output files are written to disk on every
incremental build regardless of whether their content actually changed. This
causes downstream watchers (like Vite) to see all files as "changed" and
trigger full rebuilds instead of incremental ones.

Only files with changed content should be written to disk during incremental
builds in watch mode. This allows downstream watchers to properly detect which
files actually changed and perform efficient incremental rebuilds.

Fixes #31033

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

fix(angular): eliminate race condition in shouldWriteFile function

- Remove existsSync check before readFile to prevent race condition
- Simplify error handling by relying on try-catch for both file existence and read errors
- This makes the code more robust and thread-safe

Co-authored-by: Coly010 <Coly010@users.noreply.github.com>

chore(angular): format code with prettier after merge conflict resolution
@Coly010 Coly010 force-pushed the issue-31033-fix branch from 304e6d0 to fb8d31b Compare June 5, 2025 09:15
@Coly010 Coly010 merged commit bc01bba into master Jun 5, 2025
7 checks passed
@Coly010 Coly010 deleted the issue-31033-fix branch June 5, 2025 10:07
@github-actions
Copy link
Copy Markdown
Contributor

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 Jun 11, 2025
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.

@nx/angular - ng-packagr-lite watch mode triggers full rebuild on every file change since @nx/angular 20.4

3 participants