Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add error listeners before Auth.init() #334

Closed
wants to merge 1 commit into from

Conversation

gijswijs
Copy link

In the current setup, if you want to create an error listener, you
should create a plugin, and load them through the config with the
setting auth.plugins, like so:

plugins: ['~/plugins/auth-error-listener.js']

The plugins are loaded after the main Auth plugin itself is loaded.
The main plugin takes care of the init. So if an error occurs during
init, it's impossible to catch it with a custom error listener. As mentioned
in issue #220.

This commit adds a new config option, called errorListeners, it's used
in the same way as plugins, so:

errorListeners: ['~/utils/auth-error-listener.js']

The listener itself isn't a nuxt plugin anymore. It should export a
default function:

export default function(error) {
  console.log('error listener')
  console.log(error.response.data)
  console.log(error.response.status)
}

ErrorListeners will be loaded before init. This change should be
backwards compatible, since the old method of loading listeners through
plugins still works.

In the current setup, if you want to create an error listener, you
should create a plugin, and load them through the config with the
setting auth.plugins, like so:

`plugins: ['~/plugins/auth-error-listener.js']`

The plugins are loaded *after* the main Auth plugin itself is loaded.
The main plugin takes care of the init. So if an error occurs during
init, it's impossible to catch it with a custom error listener.

This commit adds a new config option, called `errorListeners`, it's used
in the same way as plugins, so:

`errorListeners: ['~/utils/auth-error-listener.js']`

The plugin itself isn't a nuxt plugin, so it should export a
default function:

`
export default function(error) {
  console.log('error listener')
  console.log(error.response.data)
  console.log(error.response.status)
}
`

ErrorListeners will be loaded *before* init. This change should be
backwards compatible, since the old method of loading listeners through
plugins still works.
@sobolevn
Copy link
Contributor

Awesome! 👍

@dappiu
Copy link

dappiu commented Apr 15, 2019

How can we access the Nuxt context or the $auth object inside a listener?

@gijswijs
Copy link
Author

@dappiu I don't think you can, not with the new way of instantiating error listeners.
You can still use the old method, though, and you can use them both side by side.

The old method is just a 'regular' plugin, so you can get the context of the Vue app through the plugin (more info: https://nuxtjs.org/guide/plugins/).

This new method is specifically meant to catch errors during Init of the $auth, as such I didn't think it would be important to have acces to the context or $auth. (I'm not even sure you can count on them already being constructed...)

If you really needed $auth and context in the listener, loaded through errorListeners, you would have to rewrite onError. It would require onError to pass the context and $auth to the error listener. I think that's out of scope for now, since you can always use the regular method for that. Don't you agree?

@dappiu
Copy link

dappiu commented Apr 15, 2019

Yeah, I do agree, but I don't see the difference with installing your listeners by options.plugins instead of options.auth.plugins.
Using a plugin in auth.plugins was a way to give access to $auth to your plugin. If I can't access the $auth object anyway, installing the plugin with the Nuxt standard way produces a better result because you don't have access to $auth perhaps, but you have at least access to context object (and you maybe could use that to communicate something to the rest of the app in a later stage)

But let me explain why I'm interested on this, and maybe my point of view will be clearer.
I incurred in this gotcha when I found that, when a token is present from a previous navigation, fetchUser() will be called during module initialization and before the listeners were installed. Sometimes though the token may be expired and the request receives a 401 response. Now, my requirements are: logout the user if no refresh_token is present, or launch the token refresh flow and repeat the request without the user acknowledging this. I can't do this without having access to the $auth object to check for refresh_token presence, so for now I ended up with making a custom scheme (duplicate of LocalScheme) with refresh logic built into it (but this was also required for saving the refresh_token with the local scheme, that only handled access_token)

@gijswijs
Copy link
Author

Just to clarify:
When I'm talking about installing an error listener as a plugin, I'm assuming installing it via options.auth.plugins. You need access to $auth to install the listener, so that's why.

With regards to your second part of your comment. I don't think there's a way to do this less hacky at the moment. An alternative to your solution (if this PR gets merged) could be the following:

  1. Use an error listener loaded via errorListeners to listen to the 401 error, and redirect to a logout page that automatically logs out the current user.
  2. Create a 2nd listener loaded via auth.plugins to automatically handle the refresh of the token (see this comment for more info: Feature: local strategy refresh #208 (comment))

But it's not ideal. This module is in need of somebody doing some merges of PR's.

@pi0
Copy link
Member

pi0 commented May 23, 2019

Hi @gijswijs. First of all thanks for your contribution and sorry for the late PR review.

Actually, I agree with @dappiu. We can register a normal nuxt plugin before auth module to intercept axios. With #234 we now handle init > mounted errors with error handles so that should not be a concern anymore.

I'm closing this PR by assuming #234 will cover it. BTW open to discuss if your requirement is not covered.

Thanks.

@pi0 pi0 closed this May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants