RunesReforged Optional Versions

[cnguy]

by @raitono:

 I've run into an issue related to the above optional version() call for DDragon requests.
The runes reforged endpoint errors out because the realms endpoint doesn't have a version for it.
I've tracked it down to the error handling around the version,
would you like me to include the information here or open a new issue for it?

I'll add some error logs later. Thank you to @raitono for the report.

[raitono]

This is a tricky one to follow, so I will do my best to lay out the sequence of events stating with the first failure.

The process throws the expected error about requiring a version since it cannot look it up using the realms endpoint: https://github.com/cnguy/kayn/blob/fe340482bd51cb16b16422ac3c4ccccceb507812/lib/RequestClient/DDragonRequest.js#L47-L53

This error makes its way up to the try/catch block where it calls the error function: https://github.com/cnguy/kayn/blob/fe340482bd51cb16b16422ac3c4ccccceb507812/lib/RequestClient/DDragonRequest.js#L252-L255

The error function then passes a NULL data parameter to the callback at: https://github.com/cnguy/kayn/blob/fe340482bd51cb16b16422ac3c4ccccceb507812/lib/RequestClient/DDragonRequest.js#L178-L181

It then errors a second time when it cannot destructure the NULL value. This is the error that the user sees, instead of the proper version error from the beginning.

After fiddling with it for a while, the easiest solution I could come up with was simply removing both of the try/catch blocks and letting the original error make its way back to the user. This resulted in me getting the version error that I would expect and stopped the detructuring callback from being called multiple times. If we wanted to make it actually work with an optional version we could perhaps have a fallback to use the latest version gotten from the versions endpoint. This would be useful since that was my expected behavior.

[cnguy]

Awesome... the Realms endpoint seems to contain the old rune/mastery versions as well.. I wonder if there is an issue that is already raised..

  { item: '9.16.1',
     rune: '7.23.1',
     mastery: '7.23.1',
     summoner: '9.16.1',
     champion: '9.16.1',
     profileicon: '9.16.1',
     map: '9.16.1',
     language: '9.16.1',
     sticker: '9.16.1' },

as well... Obviously, if we wanted a hacky implementaiton, we could just tell it to use the same version as the others, but the versions can differ, too, so that'd probably be buggy..

Here is a comment I found on Discord:

I suppose we could just use the latest version, or just bubble up the error while also noting that optional versions does not work specifically for this endpoint. Note how there's no error checking here. Perhaps I can just re-throw the error (I forgot if this will get caught by an internal try/catch, so I'll check later).

 self.execute(url, true, function(err, data) { 
    if (err) { throw error }
     const { n: versions } = data 
     executeWithVersion(endpoint, versions) 
 }) 

[raitono]

It does get caught by your internal try/catch block. It runs that block multiple times. The first time through, you have the data set to the proper object above, so it can destructure it. executeWithVersion errors properly once it cannot find the runesReforged endpoint. That gets caught by the error handler, which calls the callback again. This time, with data being NULL. That is the error that finally gets passed back up. If we remove or refactor the error handling, the proper error will bubble up.