search

Danacus / node-factorio-api

Download and update mods from the Factorio Mod Portal
3 stars 2 forks source link

Used version of fs-jetpack has a bug in existsAsync that breaks mostly everything #10

Closed chrisgbk closed 6 years ago

chrisgbk commented 6 years ago

Per title; fs-jetpack promisifies the fs library, BUT the version in use by node-factorio-api calls the promisified fs.stat using the callback syntax, which silently breaks pretty much Everything. Need a newer version dependency. Once it gets patched - the newest version is still broken, just in a different way.

Danacus commented 6 years ago

Hmmm... Interesting. I should really update fs-jetpack. I'm kind of aware of the fact that I didn't update any dependencies in a long time, but I didn't know about the bug. Anyway, I won't update the current branch on GitHub, because I moved to Gitlab on which I have created a new branch with a complete rewrite. I don't remember using existsAsync during the code rewrite, but I'll try to remember to update fs-jetpack, because it's severely outdated. The new code should be working, but I have done a lot of testing. I'll soon either make sure the new version is stable or fix the current version.

Amd, btw, how exactly does it break everything actually? I've never noticed it... And are you saying that the newest version is also broken? In that case I will make sure not to use existsAsync.

Danacus commented 6 years ago

Looked at the issue over at the fs-jetpack repository. I see what the problem is now, but does this count for all versions of node (8, 9 and 10)? (I should probably just look that up before asking)

chrisgbk commented 6 years ago

The bug was introduced in fs-jetpack 0.13 when they removed external library dependencies and used their own internal implementation. Depending on the development timeline, if you initially wrote against 0.12, it would have worked fine.

I run node 10, it took a lot of time to hunt down (I was attempting to run factorioClusterio, which requires a few patches to run on 10 already), but 10 behaves a lot differently than 8, so I imagine, even though the functionality of existsAsync was compromised, it outwardly may appear to work on 8 due to differences in promise handling.

fs-jetpack 1.3.1 is out now that fixes this, so updating dependencies should be enough now.

Danacus commented 6 years ago

Okay, I'll do that. I'm still using node 8, because some modules don't work with node 10. I feel like it's better to wait for node 10 to become stable, but I might try to make it work with node 10.

Danacus commented 6 years ago

Upgraded all packages for both the current version and for the new version (on GitLab). I'm now using version 2.1.0 of fs-jetpack, though I personally still haven't noticed any difference.

I should really make a habit of upgrading my dependecies every so often.