search

internetarchive / dweb-mirror

Offline Internet Archive project
https://www-dweb-mirror.dev.archive.org/
GNU Affero General Public License v3.0
256 stars 27 forks source link

fix stopServer and allow a shutdown if completed #330

Closed ferm10n closed 3 years ago

ferm10n commented 3 years ago

I tried running the process (probably without proper configuration) and noticed it got stuck in the stopServer because the callback is never called when the server isn't started.

This is mainly to see if you're accepting PRs for this project. I'm thinking about getting pretty involved with it. If I'm going to start working with it, I thought I should try to see if I can contribute back. Although I'm not sure the most effective way to approach this and coordinate things.

I see you have node 12 in the dockerfile. I think this project could get a lot more support if it was more modernized, especially with handling async behavior (which currently impedes readability imo). I'm planning on making things more readable and adding type definitions for improved intellisense, as well as fixing bugs.

Anyway, I think this project is a fantastic idea and I hope I'll be able to contribute!

ferm10n commented 3 years ago

Are you talking about this line that was removed? https://github.com/internetarchive/dweb-mirror/pull/330/commits/22ce868e2ed35a9001cfb835fa11566a111284bf#diff-8ef47db7886b95bb652f29624583a93f13d2c35a52a39fbcc7abcc86426035d2L317

I actually added that in, then realized it was a mistake and corrected it. In any case though, I usually try to rely on commenting things out as a temporary thing, and relying on the commit history if something needs to be reinstated.

When was the process supposed to close? When it gets a ctrl-c from the user?

mitra42 commented 3 years ago

When was the process supposed to close? When it gets a ctrl-c from the user?

Depending on the options, this can run as a crawler (crawling archive resources to directories), or as a server or both (see the README.md)

ferm10n commented 3 years ago

Ah, my bad. I should have noticed the yarn.lock file. I've amended my commit to remove the package-lock.json.

Since you mentioned needing the process to exit, does it makes sense to add back that process.exit(0)? Maybe after this line

mitra42 commented 3 years ago

I merged the request ...

I don't think we need to add the process.exit(0) unless we see this problem. I'm pretty sure I used to have it here, but I must have removed it when the problem went away.

mitra42 commented 3 years ago

I'm presuming you are getting your releases from git, if you are getting them from npm and need me to bump the version and publish a release just let me know.