Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

Fix: watch local modules other bundles, 5.x.x #1368

Closed
wants to merge 11 commits into from

Conversation

PixnBits
Copy link
Contributor

@PixnBits PixnBits commented Apr 4, 2024

Description

During development use filesystem polling to detect modules being built and then reload them in the running server. This increases the filesystem and CPU usage, but is much more reliable.

This also adds some log entries for the user to know when a module was reloaded, removing guesswork.

Some unnecessary disk reads (module map) were removed.

Motivation and Context

Users are hitting development scenarios where new builds of modules are not detected and then not reloaded in the running server. The limitations of the Node.js built-in API are important, but are smoothed over by chokidar. However, even chokidar is not accurately emiting events, sometimes due to superseding situations like the modules being docker volume mounts, e.g. https://forums.docker.com/t/file-system-watch-does-not-work-with-mounted-volumes/12038.

How Has This Been Tested?

I've both

Before these changes running on metal would occasionally miss the file writing finishing and not reload, and running in a docker container every change after the first build would be missed. After the changes in this PR I saw every write finish detected.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (adding or updating documentation)
  • Dependency update
  • Security update

Checklist:

  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • These changes should be applied to a maintenance branch. (This is that PR, see Fix: watch local modules other bundles #1370)
  • This change requires cross browser checks.
  • Performance tests should be ran against the server prior to merging.
  • This change impacts caching for client browsers.
  • This change impacts HTTP headers.
  • This change adds additional environment variable requirements for One App users.
  • I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using One App?

Increased confidence when changes are loaded, and end (or at the very least, greatly decrease) the users having to restart the one-app server to see server bundle changes loaded.

Copy link
Contributor

github-actions bot commented Apr 4, 2024

Size Change: 0 B

Total Size: 694 kB

ℹ️ View Unchanged
Filename Size
./build/app/app.js 159 kB
./build/app/app~vendors.js 401 kB
./build/app/runtime.js 7.07 kB
./build/app/service-worker-client.js 7.25 kB
./build/app/vendors.js 120 kB

compressed-size-action

@@ -17,54 +17,165 @@
// This file is only used in development so importing devDeps is not an issue
/* eslint "import/no-extraneous-dependencies": ["error", {"devDependencies": true}] */

import fs from 'fs';
import path from 'path';
import chokidar from 'chokidar';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if this is the only place chokidar is used (directly), can we remove it from package.json?

Comment on lines 90 to 105
// this may be an over-optimization in that it may be more overhead than it saves
const stating = new Map();
function stat(filePath) {
if (!stating.has(filePath)) {
stating.set(
filePath,
fs.stat(filePath)
.then((fileStat) => {
stating.delete(filePath);
return fileStat;
})
);
}

watcher.on('change', async (changedPath) => {
try {
if (!changedPath.endsWith('.node.js')) return;
return stating.get(filePath);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

too much? (remove?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in 7398793

@PixnBits PixnBits marked this pull request as ready for review April 4, 2024 19:06
@PixnBits PixnBits requested review from a team as code owners April 4, 2024 19:06
@PixnBits

This comment was marked as resolved.

@PixnBits PixnBits force-pushed the fix/watchLocalModules-other-bundles_5.x.x branch from 7398793 to acefc8f Compare April 4, 2024 19:12
@10xLaCroixDrinker 10xLaCroixDrinker deleted the fix/watchLocalModules-other-bundles_5.x.x branch May 3, 2024 20:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants