Implement the onLoggedOut hook and add unit tests

[stevenhornung]

• Implemented the onLoggedOut hook for logout
• Updated the onLoggedIn hook to only append data.extra when a non-empty object was returned (instead of always appending data.extra)
• Added unit tests for onLoggedOut hook
• Moved onLoggedIn hook tests to same file as onLoggedOut hook tests
• Updated README for onLoggedOut hook data.extra

[stevenhornung]

@kahmali let me know if you see any issues with this PR or need anything else done to get this merged.

Thanks!

[kahmali]

I'm so sorry for the delay on this. I'm the worst. I'll get this reviewed (and hopefully merged in) tonight.

[kahmali]

Why are we requiring the extra data be an object? A user may just want to return a string there; I won't judge :P I feel like we should try to avoid being opinionated here and just let the user return any defined value. If this was because of my unnecessary return of an empty object in the default onLoggedIn method, I apologize for that terrible code.

Since the return value wasn't being used until very recently, and the empty object I return is more of a bug than a feature, I think it would be safe to change the default onLoggedIn and onLoggedOut functions to being undefined, do an existential check here like we had before, and add another to ensure the function isn't called unless the user overrides it in the API config. Putting that together should look something like:

extraData = self._config.onLoggedIn?.call(this)
if extraData?
  _.extend(response.data, {extra: extraData})

And of course my useless onLoggedIn and onLoggedOut functions will need to be removed from the default config in the Restivus constructor (and the docs updated to reflect that those functions are undefined by default).

What do you think of that approach?

[stevenhornung]

@kahmali Exactly right. I didn't feel like always returning an empty object was the right thing to do, hence the empty obj check. I think the adjusted approach suggested is better, though.

So to summarize:

1. Remove function declarations from constructor (where they return an empty obj by default)
2. Change extraData check back to just check if something was returned and remove empty obj check (so anything can be returned)

Simple enough and makes sense to me. Let me know if I should just create a new PR and close this one or just push to this one with an updated commit, or ?.

Thanks!

[kahmali]

Your summary is accurate, except one more thing:
3. Do an existential check to make sure the function is defined before trying to call it (you may just be implying this as it would cause an error during testing once you removed my stupid functions in step 1)

You can follow any PR strategy that's easiest to you. I personally like to rebase and force push to the same branch. It keeps the conversation all in a single thread, and doesn't create "dead end" PRs. Just my personal preference, though. There are worse things in the world than a few dead PRs.

[kahmali]

Here's what I'm referring to, in code form (from my code example above):

extraData = self._config.onLoggedIn?.call(this)

Notice my sneaky existential check of the onLoggedIn function there.

[stevenhornung]

Alrighty. I just pushed that change to not force data.extra to return an object. I also added that null check on the hook calls so it doesn't throw any errors after removing the function definitions from the constructor. Let me know if all this looks good! Thanks

[kahmali]

This looks great! I'm gonna go ahead and merge this in and publish the update. I'll let you know when it's all set.

Thanks for taking care of this!

[kahmali]

I updated the change log and published this in v0.8.10.

Thanks again for all your help with this!