search

ThauEx / ffrk-proxy

Proxy for Final Fantasy Record Keeper written in node.js
GNU Lesser General Public License v3.0
39 stars 21 forks source link

Add last stand, astra and ex: soldier mode. #89

Closed jdel closed 7 years ago

jdel commented 7 years ago

Implement #82

SimonOmega commented 7 years ago

I tried to make a pull off your pull... But it is not working... I don't think I know how to do it properly. I added a few more status IDs, and created some detailed descriptions. Also corrected Stan to Stun.

ffrk-proxy.zip

ThauEx commented 7 years ago

As far as I know 'stan' is no typo, the name got extracted from the game files.

SimonOmega commented 7 years ago

Ohh, I was looking at a posted dump. I didn't dump it from the game myself. Maybe they edited it to match the in-game description. I noticed a few of their IDs were off as well.

Didn't mean to insult your typing, sorry about that.

jdel commented 7 years ago

I'll try and have a look at this

SimonOmega commented 7 years ago

Didn't mean to cause trouble if I did.

I tested them last night, but a second set of eyes is always welcome.

ThauEx commented 7 years ago

Since I'm not playing anymore, I cannot check... Maybe they fixed the typo, I don't know. We could add both. Just tell me, when the PR is ready to get merged.

ThauEx commented 7 years ago

@jdel Do you want to change something or can I merge it?

jdel commented 7 years ago

Sorry, been pretty busy lately. I will find some time this week to review the dump.

jdel commented 7 years ago

Sorry for the delay, this should be the final PR.

I had to add thin in packages.json otherwise npm install would uninstall it. Not sure why.

There is a docker image for those who want to test: jdel/ffrk-proxy:pr89

ThauEx commented 7 years ago

Thanks for your work, there is one small thing: The proxy uses a modified version of thin, so installing the original one would break everything. I could refactor this, so the lib is not in node_modules anymore. Then you just need to remove the dependency again, are you fine with that?

ThauEx commented 7 years ago

Any update?

jdel commented 7 years ago

Hi, sorry, had completely forgotten this was still pending.

I am not entirely sure what has been changed in thin, but i've been running the docker container jdel/ffrk-proxy:pr89 with the original thin since early october without any issue.

Maybe there is just a parameter to give to npm install so it doesn't remove the custom thin ? This is how it worked in the past. I don't know why npm removes modules now.

jdel commented 7 years ago

Ok, so i have just done a couple of tests, with node 5.x or 6.x and npm 3.x, npm install will leave custom thin in node_modules. While 8.x and 9.x (with npm 5.x) are problematic.

I am happy to remove thin from packages.json and update documentation that node version should be <= 6.x and npm version <= 3.x

ThauEx commented 7 years ago

Thanks for your response. To avoid these node js issues, I will push an update in the next days to fix that.

ThauEx commented 7 years ago

Done with c83984abc177e69e3490fc1cde120836a2952cec

ThauEx commented 7 years ago

Great! Can I merge it?

jdel commented 7 years ago

LGTM !

jdel commented 7 years ago

Can you create a release too so I can amend my https://github.com/jdel/docker-ffrk-proxy repo ?

ThauEx commented 7 years ago

Sure, I will do it later today.

ThauEx commented 7 years ago

Okay, release has been created.

jdel commented 7 years ago

Docker image is ready https://hub.docker.com/r/jdel/ffrk-proxy/tags/