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

Config format #66

Closed
totaltrash opened this issue Feb 12, 2018 · 2 comments
Closed

Config format #66

totaltrash opened this issue Feb 12, 2018 · 2 comments

Comments

@totaltrash
Copy link
Contributor

totaltrash commented Feb 12, 2018

Hi, I'm loving the changes for 4.0, but I feel the config structure could use a little work to better group options. I was thinking about structuring the options based on the 'actions' (login, logout, fetch user - a bit like v3), but rolling in the redirects with each action - for me there is a bit of confusion about how the redirects work, and thought something like this would be a bit more explicit to configure. Default options could look something like this:

module.exports = {
  login: {
    endpoint: { url: '/api/auth/login', method: 'post', propertyName: 'token' },
    autoFetchUser: true,
    redirect: '/',
    rewriteRedirect: true
  },
  logout: {
    endpoint: { url: '/api/auth/logout', method: 'post' },
    redirect: '/login',
    autoClearHeader: true
  },
  fetch_user: {
    endpoint: { url: '/api/auth/user', method: 'get', propertyName: 'user' }
  }
  resetOnError: false,
  watchLoggedIn: true,
  namespace: 'auth',
  scopeKey: 'scope',
  token: {
    type: 'Bearer',
    name: 'token'
  },
  cookie: {
    name: 'token',
    options: {
      path: '/'
    }
  }
}

I added an autoClearHeader option to the logout action (which would be the logical place to configure a fix for #57). I'm not 100% sure what watchLoggedIn actually does, but that could be moved to the login and/or logout options... Anyway, thought I would throw this out there!

This question is available on Nuxt.js community (#c49)
@breakingrobot
Copy link
Contributor

breakingrobot commented Feb 12, 2018

@totaltrash

watchLoggedIn is a watcher for loggedIn which will redirect you automatically on login and logout, as you can implement your own watcher logic, we added an option to disable this watcher, though it could be renamed again to be clearer 👍

It is not directly related to login or logout, so imho, it should not be in one or both of these 😄

I am not agreeing with your structure proposal which would mostly be a rollback to 3.0 which was:

const defaults = {
  user: {
    endpoint: 'auth/user',
    propertyName: 'user',
    resetOnFail: true,
    enabled: true,
    method: 'GET'
  },
  login: {
    endpoint: 'auth/login',
  },
  logout: {
      endpoint: 'auth/logout',
      method: 'GET',
  },
  redirect: {
    notLoggedIn: '/login',
    loggedIn: '/'
  },
  token: {
    enabled: true,
    type: 'Bearer',
    localStorage: true,
    name: 'token',
    cookie: true,
    cookieName: 'token'
  }
}

Structuring only by actions was what was done in 3.x and have been revised to a modular endpoint structure which will benefit us in the future.

For further explanation :

  • rewriteRedirect should not be moved on login while this is a global concern, that does not apply on to login
  • endpoints structure being replaced by an action structure is a big no-go to me, we added this section to concentrate every endpoints and use OAuth/strategies endpoints in the future, that would completely break forward-compatibility with old release-candidates and limit us in the future.
  • autoFetchUser should be global and fetchUserOnLogin is a bad name too because the module does fetch the user on others situations.

I am formerly disagreeing with your proposal, but I am leaving this discussion open for others to add their concerns to this debate.

I find current the current structure to be a good fit between modularity and explicitness, but could be clearer and have better options naming.

@totaltrash
Copy link
Contributor Author

Thanks @breakingrobot that all makes sense,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants