Skip to content

Conversation

@allusernamestakenexceptthis
Copy link

@allusernamestakenexceptthis allusernamestakenexceptthis commented Jun 18, 2024

πŸ”— Linked issue

Similar to the fix introduced in latest, but this for the 0.7 version
can incorporate this as v0.7.3?

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Without this fix, sidebase/auth wipes the nuxt config types causing type errors

Copied from original PR:
Resolves #797

This moves the type augmentation to module.ts and exports the ModulePublicRuntimeConfig interface (as well as ModuleOptions) which are used by nuxt/module-builder to generate the correct types.

The original type augmentation is kept to for type inference in the source code (matches previous behaviour).

Without these changes the types will conflict with those of other modules (nuxt-modules/i18n#2915), I have tested these changes locally with the reproduction provided in the referenced issues and it seems to work.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

@allusernamestakenexceptthis allusernamestakenexceptthis changed the title Update module 0.7.3 fix(types): export module builder type generation to avoid wiping configs Jun 18, 2024
@allusernamestakenexceptthis allusernamestakenexceptthis marked this pull request as ready for review June 18, 2024 17:16
@zoey-kaiser
Copy link
Member

Hi @allusernamestakenexceptthis πŸ‘‹

Thank you for your PR! Sorry for the short delay, @phoenix-ru and I were both taking a vacation. Could you please resolve the merge conflicts and we can then move onto a review 😊

@zoey-kaiser zoey-kaiser added p4 Important Issue bug A bug that needs to be resolved labels Jun 27, 2024
@allusernamestakenexceptthis
Copy link
Author

@zoey-kaiser

no problem. hope you had a good vacation ( :

regarding conflicts. I'm suggesting this changes on top of 0.7.2 not latest version 8 alpha. I think that requires creating 0.7.2 branch on your end. and then we change pull request target to be against 0.7.2. would that be possible?

*I haven't worked much with tags, so if I got it wrong, please let me know.

@zoey-kaiser
Copy link
Member

regarding conflicts. I'm suggesting this changes on top of 0.7.2 not latest version 8 alpha. I think that requires creating 0.7.2 branch on your end. and then we change pull request target to be against 0.7.2. would that be possible?

I understand your point! I would pass this question onto @phoenix-ru, who manages most of the code rewrites in 0.8.0. We generally want to avoid running multiple branches / tags in the project (as it does get pretty messy). Would your PR only apply to release < 0.8.0 or could later versions also benefit from these changes?

@allusernamestakenexceptthis
Copy link
Author

I understand your point! I would pass this question onto @phoenix-ru, who manages most of the code rewrites in 0.8.0. We generally want to avoid running multiple branches / tags in the project (as it does get pretty messy). Would your PR only apply to release < 0.8.0 or could later versions also benefit from these changes?

@zoey-kaiser

this fix only for 0.7 version. It is already fixed in 0.8.0. The problem it might be sometime before 0.8 is ready for general production. Currently we are on 0.7.2 and have to work around type issues when using nuxt config due to this bug.

I understand the mess with running multiple branches, but would be awesome to temporarily have one to update 0.7, might be even a good idea to keep one stable for bug fixes for stable channel

thanks

@zoey-kaiser
Copy link
Member

this fix only for 0.7 version. It is already fixed in 0.8.0. The problem it might be sometime before 0.8 is ready for general production. Currently we are on 0.7.2 and have to work around type issues when using nuxt config due to this bug.

Hi @allusernamestakenexceptthis πŸ‘‹

0.8.0 is already pretty close to being ready for production use! We have completed our scope for the 0.8.0 release now, and I have just published the first release candidate (https://github.com/sidebase/nuxt-auth/releases/tag/0.8.0-rc.1). We had this release sit in alpha for a long time, as we had significant rewrites in the types and internal systems of NuxtAuth. We have been testing 0.8.0 for the last month in multiple internal projects and feel confident it is stable!

We plan to test the release candidate next week and aim for a full release next week. Would you still like to continue pushing your patch, even with this timeline? My concern is that our release pipeline is pretty simple, and retroactively pushing a change for an older version is tricky. We are planing to revamp our release pipeline after 0.8.0, to account for cases such as this, however at the moment it sadly does not allow for it.

If you still require this change I would therefore recommend either:

  • Publishing your fork
  • Using Patch Package to fix the issue until the full release of 0.8.0

Please let me know your thoughts 😊

@allusernamestakenexceptthis
Copy link
Author

@zoey-kaiser

Thanks for the details especially the status of version 0.8.0

indeed, with this timeline, it's better to wait. We'll try the RC in development. and if there is any issue will report them here. we might go with this for production too.

I'll close this.

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

Labels

bug A bug that needs to be resolved p4 Important Issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants