ExpeditionRPG / expedition

Expedition: The Cards & App RPG
https://ExpeditionGame.com
Other
79 stars 26 forks source link

Handle api downtime #822

Closed Lordtien closed 4 years ago

Lordtien commented 4 years ago

Fixes #815

toddmedema commented 4 years ago

@Lordtien looks like there's a typescript error with your change (you can see it by clicking on the "details" next to the red X in the "All checks have failed" section:

ERROR in [at-loader] ../app/src/components/views/ModeSelectContainer.tsx:11:3 
    TS2322: Type '{ isLatestAppVersion: boolean | undefined; settings: SettingsType; user: UserState; multiplayer: ...' is not assignable to type 'StateProps'.
  Types of property 'isLatestAppVersion' are incompatible.
    Type 'boolean | undefined' is not assignable to type 'boolean'.
      Type 'undefined' is not assignable to type 'boolean'.
Lordtien commented 4 years ago

@Lordtien looks like there's a typescript error with your change (you can see it by clicking on the "details" next to the red X in the "All checks have failed" section:

ERROR in [at-loader] ../app/src/components/views/ModeSelectContainer.tsx:11:3 
    TS2322: Type '{ isLatestAppVersion: boolean | undefined; settings: SettingsType; user: UserState; multiplayer: ...' is not assignable to type 'StateProps'.
  Types of property 'isLatestAppVersion' are incompatible.
    Type 'boolean | undefined' is not assignable to type 'boolean'.
      Type 'undefined' is not assignable to type 'boolean'.

@toddmedema While dispatching, we need to pass isLatestAppVersion. Is it a good idea to pass that as false, since if we have a server error, we cannot be sure?

toddmedema commented 4 years ago

So, thinking about it more, I acfhaklg5 like your original approach of leaving it as undefined - because we really don't know, since there was a server error.

Having it be undefined just means that you need to check to see if it's defined before it's referenced, i.e. if (typeof yourvar !== 'undefined')

Lordtien commented 4 years ago

So, to me, my initial method seemed like a hack, since... 1) Taking more than expected out of a boolean, i.e. isLatestAppVersion 2) Might break code at other places.

Suggestions: 1) Keep it false, but adds another thing to remember. 2) Add a new property, if we expect to use this in the future, like, isConnected or isServerOnline.

For both, I'll be exploring the places we're using this as of now.

Lordtien commented 4 years ago

In initialServerStatusState, we do put in isLatestAppVersion as false, since it's unknown at that point in time. I think we can still do the same so as to keep the logic consistent. On the other hand, if we go with keeping it undefined, then we need to make changes for the new possible value as well as comment it at the definition so that it can be read. IMO, the second idea still sounds like a hack as we're pushing the application of a boolean and it adds to the "Things to remember". @toddmedema Need your comments before proceeding further.

toddmedema commented 4 years ago

Good brainstorming! Let's do serverOffline as a new field, so that we can succinctly check if the server is definitely not available via if (serverOffline)

Lordtien commented 4 years ago

Alright. Adding it as an optional field with false as a default value. Once the check fails, or maybe if the server doesn't respond mid-way, we can set it to true.

Keeping isLatestAppVersion as false for this case.