Fix: watch local modules other bundles #1370
Conversation
e.g. https://forums.docker.com/t/file-system-watch-does-not-work-with-mounted-volumes/12038/5\?page\=2 CHOKIDAR_USEPOLLING not reliable either somehow
inconsistentcies in the event loop evaluation, so switched to manual timer mocks
|
Size Change: 0 B Total Size: 232 kB ℹ️ View Unchanged
|
| return; | ||
| } | ||
|
|
||
| console.log(`the Node.js bundle for ${moduleName} finished saving, attempting to load`); |
There was a problem hiding this comment.
As per the logging pr, can this be console.info?
We should only put 1 lot out in general for notifying the user that app has detected a change to a local module. The success, or the error.
There was a problem hiding this comment.
depends, if the module takes a long time to load it would be useful to the user to know that the process has started
There was a problem hiding this comment.
regarding the logging level, IDK if the change is useful here? #1372 (comment)
There was a problem hiding this comment.
changed the levels in d32878d
There was a problem hiding this comment.
for the log entries, maybe something like
const SAVING_NOTIFICATION_LOAD_WAIT = 500;
...
const loadNotificationHandle = setTimeout(() => {
console.info('the Node.js bundle for %s finished saving, attempting to load', moduleName);
}, SAVING_NOTIFICATION_LOAD_WAIT).unref();
...
newModule = addHigherOrderComponent(await loadModule(
...
console.info('finished reloading %s', moduleName);
clearTimeout(loadNotificationHandle);?
There was a problem hiding this comment.
Sorry, I didn't mean for you to make both of these info
The second log, that tells the user something has been reloaded should be log log level still.
This is an important piece of information for the deverloper, and the lack of this notification is currently a point of frustration for Devs.
That said, I really like your time based de-duplicattion here. If all is well, they see 1 log saying success, if it's taking way longer than normal, they see a 'attempting to reload' message, if they then never get the success message they know something is wrong.
In that case, both of these can be log log level.
We just have to be sure that 500ms is the right time budget
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
rather than string templates, for performance
| const CHANGE_WATCHER_INTERVAL = 1000; | ||
| const WRITING_FINISH_WATCHER_TIMEOUT = 400; |
There was a problem hiding this comment.
these should be configurable by the user
I'm debating via arguments or environment variables
There was a problem hiding this comment.
perhaps we let that be another PR so as to get this fix in, and then resolve the plan?
There was a problem hiding this comment.
I'd prefer arguments. Particularly since this is just local dev. They are much more discoverable since they are self documenting. Also those using one-app-runner will be able to very easily include them in the runner config.
There was a problem hiding this comment.
What do you think of holding this until Node.js 20 is in, and using fs.watch? It should simplify a lot of this.
There are some caveats to it though.
On Windows, no events will be emitted if the watched directory is moved or renamed. An EPERM error is reported when the watched directory is deleted.
I think this one will be fine.
On IBM i systems, this feature is not supported.
I think this one might be inaccurate, based on the following release note for fs.watch
| Version | Changes |
|---|---|
| v19.1.0 | Added recursive support for Linux, AIX and IBMi. |
On Linux and macOS systems, fs.watch() resolves the path to an inode and watches the inode. If the watched path is deleted and recreated, it is assigned a new inode. The watch will emit an event for the delete but will continue watching the original inode. Events for the new inode will not be emitted. This is expected behavior.
Sounds like if we run into this issue, we may be able to listen for the delete and recreate the watcher.
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
$ npm serve-module some-moduleand run the serverBefore 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
Checklist:
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.