Refactor FileWatcher into a Class #4916
Open
+176
−141
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This stack of pull requests is managed by Graphite. Learn more about stacking. |
isaacroldan
force-pushed
the
11-25-refactor_filewatcher_into_a_class
branch
from
cca2ed9
to
4ccf516
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+237
to
+243
| private readonly close = () => { | ||
| outputDebug(`Closing file watcher`, this.options.stdout) | ||
| this.watcher | ||
| ?.close() | ||
| .then(() => outputDebug(`File watching closed`, this.options.stdout)) | ||
| .catch((error: Error) => outputDebug(`File watching failed to close: ${error.message}`, this.options.stderr)) | ||
| } |
There was a problem hiding this comment.
The close method should remove the event listener that was added to options.signal in the start method. Not removing it could cause memory leaks if the FileWatcher instance is reused, as the old listener would remain attached to the signal.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
isaacroldan
force-pushed
the
11-25-refactor_filewatcher_into_a_class
branch
from
4ccf516
to
7306041
Compare
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success1958 tests passing in 888 suites. Report generated by 🧪jest coverage report action from ff1bc64 |
packages/app/src/cli/services/dev/app-events/app-event-watcher.test.ts
Outdated
Show resolved
Hide resolved
isaacroldan
force-pushed
the
11-25-refactor_filewatcher_into_a_class
branch
from
7306041
to
d975467
Compare
2 tasks
isaacroldan
force-pushed
the
11-25-refactor_filewatcher_into_a_class
branch
2 times, most recently
from
77a4f78
to
1b4002b
Compare
|
We detected some changes at packages/*/src and there are no updates in the .changeset. |
isaacroldan
force-pushed
the
11-25-refactor_filewatcher_into_a_class
branch
2 times, most recently
from
0899271
to
4536258
Compare
isaacroldan
force-pushed
the
11-25-refactor_filewatcher_into_a_class
branch
from
4536258
to
ff1bc64
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
Refactors the file watcher implementation into a class-based structure to improve maintainability and testability of the file watching system used during app development.
WHAT is this pull request doing?
NO NEW FEATURES, mainly a refactor:
FileWatcherclassmicromatch, it's not used anymore.How to test your changes?
devcommand on an app with multiple extensionsMeasuring impact
Checklist