[PR changes #392] feat: move pointer and sessionDataType to the SessionConfig

[valh1996]

Hi,

As I don't have the rights to push directly on the original PR (#392) by @Danielwinkelmann, I'm opening a new one.

This PR includes all of @Danielwinkelmann's changes and applies @BracketJohn's latest comments.

As a reminder, here is the comment: #392 (comment)

Thus sessionDataType and the pointer are moved directly into the SessionConfig and renamed to avoid redundancy with the "Session" keyword:

session: {
    dataType: { id: 'string' },
    dataResponsePointer: '/data/user'
},

Don't hesitate @zoey-kaiser & @BracketJohn to let me know about any changes you'd like to make so that we can move forward 🙏🏻

[zoey-kaiser]

Hi @valh1996 Thanks for taking this over. Could you please resolve the merge conflicts and then ill do a review?

[valh1996]

Hi @valh1996 Thanks for taking this over. Could you please resolve the merge conflicts and then ill do a review?

Hi @zoey-kaiser, it's done, in principle it should be OK. There were many changes to merge, so I've corrected the conflicts manually.

Can you just check that the new dataType property in the SessionConfig works well with the authjs and refresh providers ? I don't really know them since I only use local.

Thanks

[valh1996]

Hi @zoey-kaiser, hope you're well. I know I'm a bit pushy, I apologize for that but I really need this PR as I'm in the middle of rewriting my application to nuxt3. Could you please release an alpha version to be able to move forward?

Once again, if anything is missing, I am available to help.

In the meanwhile, i'm using a pnpm patch.

[zoey-kaiser]

Hi! I have some time on Thursday to do a proper review. If we want to get an alpha version out first it would be great if you could answer the following questions for me, then I don't have to look into the code myself to figure it out 😓

• What migrations will we expect from people using the local provider once we release this?
• This PR allows developers to pharse the sessionData type to allow better type support right?
• Can we also add this to the new refresh provider to ensure the two providers are on a similar feature set? (I can also add this afterwards if you don't have time)

[valh1996]

No worries, I understand, plus it's an open-source project so I'll wait and thank you again for your work^^

---

• What migrations will we expect from people using the local provider once we release this?

They will have to update their nuxt.config.ts as the sessionDataType property will no longer exist in the provider config

Instead, they must define the session property as follow:

session: {
  dataType: {
    id: 'number',
    username: 'string',
  },
  dataResponsePointer: '/data',
  enableRefreshPeriodically: false,
  enableRefreshOnWindowFocus: true,
}

Please note the two new properties:

• dataType they will have to move their old sessionDataType configuration to this one
• dataResponsePointer the pointer

This PR allows developers to pharse the sessionData type to allow better type support right?

Yes, for example if an API returns something like :

{
  "data": {
    "id": 1,
    "username": "val"
  },
  "success": false
}

So with the configuration above, and in particular the pointer, my user can be returned directly, so I don't have to do data.value.data unnecessarily, but directly data.value

Can we also add this to the new refresh provider to ensure the two providers are on a similar feature set? (I can also add this afterwards if you don't have time)

Since the configuration is in the session and no longer in the provider itself, this should normally work for all providers. I will check this and correct if necessary.

[valh1996]

Hey @zoey-kaiser !
I took a little time this morning to put things in order:

1. The enableRefreshPeriodically and enableRefreshOnWindowFocus params are now optional in the nuxt.config.ts session object. Default values are used.

2. I've taken over some of the changes @Danielwinkelmann made to the jsonPointerGet method to have better typings using generics. There is NO breaking changes.

3. I've therefore specified the return type wherever the jsonPointerGet method is used, even in the refresh provider.

4. I've fixed a bug I left behind by copying Daniel's changes.

---

Since point 1 has been done, users will only have to remove the sessionDataType from the provider in the nuxt.config.ts and add the two new properties to the session object:

session: {
  dataType: {
    id: 'number',
    username: 'string',
  },
  dataResponsePointer: '/data',
  // enableRefreshPeriodically: false,   -> now optional
  // enableRefreshOnWindowFocus: true    -> now optional
}

Moreover, since the refresh provider is based on the local provider's getSession(), it works out-of-the-box.

[zoey-kaiser]

Hi @valh1996

Thank you so much for continuing to push this! I really like the direction this PR is going in. There is one more type issue, but I think we are getting close to being able to merge 🥳

Therefore I wanted to quickly give you an outlook on the next steps:

• As this is a breaking change where the config needs to be updated, I will need to do a minor release for this (instead of a patch).
• We currently have an Alpha patch released with the new refresh provider. On Thursday I will therefore do a comprehensive test of this and if everything goes well, release the patch version.
• Once this is done, I can merge this PR and make an alpha release of the next minor update!

Due to this, I will also need to check how I want to sort the docs, to ensure that there is a clear separation between the nuxt config setup from version 0.6 to the new 0.7 we would be releasing with this change.

I will discuss this once more with my team internally to see how we want to best approach this! I will keep you updated.

[phoenix-ru]

Hi @valh1996, could you please resolve the last conflict? Your PR is on our shortlist, so we want to push it

[valh1996]

Hi @phoenix-ru
I've updated the PR on the main branch, which should correct the problem. Let me know

[phoenix-ru]

@valh1996 Thanks for a quick reaction. We have your PR planned for 0.7 because it includes some breaking changes as you already know.
For that we need to ensure stability of 0.6 first. This means adding E2E tests to our CI.
Sorry for delays, we just need some infra setup first. Will try to iterate fast.

[phoenix-ru]

@valh1996 Amazing, I'll consult with @zoey-kaiser if we can merge the PR and do a new release (0.8.0) or if there are any other non-breaking changes which can be included in 0.7

[valh1996]

Hello @phoenix-ru any news on this one please?

[phoenix-ru]

Hi @valh1996 , sorry for keeping you waiting.
I don't think I can follow up this week, but I will put this to high priority for the next week.
I think our 0.7 is good already, and we already have several breaking changes to prepare 0.8, so I would try to merge this asap (not guaranteeing next week, but it's possible).

In the meantime, could you please resolve merge conflicts?

[valh1996]

sorry @phoenix-ru, I really haven't had time to look at the conflicts, no time at the moment, thanks for doing so !

[phoenix-ru]

No problem @valh1996 , I have resolved them. Sorry for keeping you waiting and good work!