RunesReforged Optional Versions #73

Activity

1. cnguy

   added

   bug

   help wanted

   on Aug 13, 2019
2. raitono commented on Aug 13, 2019

   

   raitono

   on Aug 13, 2019

   Contributor

   More actions

   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:
   

   kayn/lib/RequestClient/DDragonRequest.js

   Lines 47 to 53 in fe34048

   	case DDragonRequestTypes.CDN.DATA:
   	if (!urlInformation.version) {
   	throw new Error(`
   	[kayn]: version() is required for DDragon data requests currently.
   	e.g. kayn.DDragon.Champion.list().version('8.15.1')
   	`)
   	}

   This error makes its way up to the try/catch block where it calls the error function:
   

   kayn/lib/RequestClient/DDragonRequest.js

   Lines 252 to 255 in fe34048

   	} catch (ex) {
   	// Don't think this is useful?
   	error({ statusCode: ex.statusCode, url, error: ex })(cb)
   	}

   The error function then passes a NULL data parameter to the callback at:
   

   kayn/lib/RequestClient/DDragonRequest.js

   Lines 178 to 181 in fe34048

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

   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.
3. cnguy commented on Aug 24, 2019

   

   cnguy

   on Aug 24, 2019

   Last edited by cnguy

   OwnerAuthor

   More actions

   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) 
    }) 
   
4. raitono commented on Aug 24, 2019

   

   raitono

   on Aug 24, 2019

   Contributor

   More actions

   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.