Skip to content
This repository was archived by the owner on Apr 6, 2020. It is now read-only.

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

Merged
ThauEx merged 5 commits intoThauEx:masterfrom
jdel:patch-1
Nov 21, 2017
Merged

Add last stand, astra and ex: soldier mode.#89
ThauEx merged 5 commits intoThauEx:masterfrom
jdel:patch-1

Conversation

@jdel
Copy link
Copy Markdown
Contributor

@jdel jdel commented Sep 18, 2017

Implement #82

@SimonOmega
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Owner

ThauEx commented Sep 19, 2017

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

@SimonOmega
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Contributor Author

jdel commented Sep 19, 2017

I'll try and have a look at this

@SimonOmega
Copy link
Copy Markdown

Didn't mean to cause trouble if I did.

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

@ThauEx
Copy link
Copy Markdown
Owner

ThauEx commented Sep 20, 2017

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
Copy link
Copy Markdown
Owner

ThauEx commented Sep 28, 2017

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

@jdel
Copy link
Copy Markdown
Contributor Author

jdel commented Oct 2, 2017

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

@jdel
Copy link
Copy Markdown
Contributor Author

jdel commented Oct 7, 2017

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
Copy link
Copy Markdown
Owner

ThauEx commented Oct 9, 2017

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
Copy link
Copy Markdown
Owner

ThauEx commented Nov 16, 2017

Any update?

@jdel
Copy link
Copy Markdown
Contributor Author

jdel commented Nov 17, 2017

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
Copy link
Copy Markdown
Contributor Author

jdel commented Nov 17, 2017

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

@jdel jdel changed the title Add last stand, astra and ex: soldier mode. WIP: Add last stand, astra and ex: soldier mode. Nov 17, 2017
@ThauEx
Copy link
Copy Markdown
Owner

ThauEx commented Nov 19, 2017

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

@ThauEx
Copy link
Copy Markdown
Owner

ThauEx commented Nov 20, 2017

Done with c83984a

@ThauEx
Copy link
Copy Markdown
Owner

ThauEx commented Nov 21, 2017

Great! Can I merge it?

@jdel
Copy link
Copy Markdown
Contributor Author

jdel commented Nov 21, 2017

LGTM !

@jdel
Copy link
Copy Markdown
Contributor Author

jdel commented Nov 21, 2017

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

@jdel jdel changed the title WIP: Add last stand, astra and ex: soldier mode. Add last stand, astra and ex: soldier mode. Nov 21, 2017
@ThauEx
Copy link
Copy Markdown
Owner

ThauEx commented Nov 21, 2017

Sure, I will do it later today.

@ThauEx ThauEx merged commit 9836787 into ThauEx:master Nov 21, 2017
@ThauEx
Copy link
Copy Markdown
Owner

ThauEx commented Nov 21, 2017

Okay, release has been created.

@jdel jdel deleted the patch-1 branch November 22, 2017 10:45
@jdel
Copy link
Copy Markdown
Contributor Author

jdel commented Nov 22, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants