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!: new ReaderWatch class for handling deleted or replaced files #27

Merged
merged 26 commits into from
Oct 17, 2024

Conversation

gmaclennan
Copy link
Member

@gmaclennan gmaclennan commented Oct 17, 2024

BREAKING: you can no longer start the server with a filepath that does not point to a valid SMP file. If you want this functionality, use the new ReaderWatch class and pass an instance of that to the server.

The ReaderWatch class will watch the given filepath to see if the file is deleted or replaced. Pass an instance of ReaderWatch to the server to support serving a fixed filepath which may or may not have a file, and watch for if the file at the filepath changes.

@gmaclennan gmaclennan self-assigned this Oct 17, 2024
* main:
  feat!: Remove lazy option for server (#30)
@gmaclennan gmaclennan changed the title feat!: handle file deletions and replacements in server.js feat: handle file deletions and replacements in server.js Oct 17, 2024
@gmaclennan gmaclennan changed the title feat: handle file deletions and replacements in server.js feat!: handle file deletions and replacements in server.js Oct 17, 2024
@gmaclennan gmaclennan changed the title feat!: handle file deletions and replacements in server.js feat!: new ReaderWatch class for handling deleted or replaced files Oct 17, 2024
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

Overall lgtm. Had a couple of non-blocking remarks, a couple of which I'll address before merging.

lib/server.js Outdated Show resolved Hide resolved
lib/reader-watch.js Show resolved Hide resolved
test/server.js Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

one thing that's a nice-to-have here is making sure that registering with a prefix works properly. Thought there used to be tests for that but maybe they got lost with the recent changes?

Not blocking and can do as a follow-up

@achou11 achou11 merged commit 3a83caf into main Oct 17, 2024
9 checks passed
@achou11 achou11 deleted the feat/watch-file-changes branch October 17, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants