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

Bump dependencies and add ModuleOptionsNormalized type #665

Merged
merged 6 commits into from
Feb 22, 2024

Conversation

phoenix-ru
Copy link
Collaborator

@phoenix-ru phoenix-ru commented Feb 15, 2024

Addresses known security advisories. Strengthens the type safety of the module.

Closes #660

@phoenix-ru
Copy link
Collaborator Author

Relevant PR: #668

Since current PR is about dependencies, I decided to include the change done there to the current PR. #668 can be closed when this is merged

@codetheorist
Copy link

So you wasted your own time adding the 660 changes to your PR when you could have merged mine and had me labelled as a contributor, seeing as though a create issues, comment on issues and do TRY to fix issues.

@codetheorist
Copy link

I've closed that PR anyway, I won't waste my own time in future.

Copy link
Member

@zoey-kaiser zoey-kaiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, however let's make a pre-release and verify the changes work in the nuxt-auth-example!

@codetheorist
Copy link

Approved, however let's make a pre-release and verify the changes work in the nuxt-auth-example!

Would it be worth reopening my original OR for the VueUse issue? It seems beneficial and is a very minimal PR with minimal testing required so could be merged quite quickly.

@phoenix-ru
Copy link
Collaborator Author

@codetheorist I see your point. However, I want to push a more general dependency revamp so that it unlocks the other PRs.
We have some breaking changes planned for 0.7 and therefore we need to make 0.6 as stable as possible, which also includes adding E2E tests.

There's nothing wrong with your PR.

@phoenix-ru phoenix-ru merged commit 2098f39 into main Feb 22, 2024
4 checks passed
@phoenix-ru phoenix-ru deleted the feature/bump-dependencies-and-type-options branch February 22, 2024 12:35
@phoenix-ru phoenix-ru mentioned this pull request Feb 22, 2024
4 tasks
@codetheorist
Copy link

That in no way stated why my PR couldn't be merged and also didn't negate the fact that I'd already done the work, if even only 15 minutes (including searching for installs, update lock files), which was rendered pointless as you decided to do it again after seeing my already open PR.

@codetheorist
Copy link

It's done now anyway but I'll be more reluctant to devote my time to issues and PRs within this repository moving forward. When people try to contribute, the least you can do is let them get the "contributor" badge to acknowledge them as such.

@zoey-kaiser
Copy link
Member

That in no way stated why my PR couldn't be merged and also didn't negate the fact that I'd already done the work, if even only 15 minutes (including searching for installs, update lock files), which was rendered pointless as you decided to do it again after seeing my already open PR.

Hi @codetheorist 👋
Sorry for not merging or prioritizing your PR. We are very thankful for your contribution and the only reasons why we decided not to merge your PR was:

  • This PR also changed a lot of packages, which greatly changed the pnpm-lock.json. Resolving merge conflicts within this file is normally very tedious and did not make a lot of sense, when we have two PRs open for this propose.
  • removing the vue-use package is not blocking anyone from continuing to use NuxtAuth. I agree that we should definitely remove dead-code, but I see no reasons to prioritize it so far, that we merge it prior to breaking changes and make a dedicated release for it.

It's done now anyway but I'll be more reluctant to devote my time to issues and PRs within this repository moving forward. When people try to contribute, the least you can do is let them get the "contributor" badge to acknowledge them as such.

I think it is great that you are trying to contribute to open source projects, however I think that you also need to see this from our perspective. Our main goal is to continue developing NuxtAuth and we really appreciate help from outside contributors, however, if mergeing other PRs causes more work for us, then it is not worth it. I would also want to point out that this moment, that this PR was already opened, before you created your PR (taken, it did not include removing vue-use), but IMO it makes a lot more sense to bundle bigger adjustments to packages, due to the reasons mentioned above.
We could have announced this earlier, but I would still like to ask you to respect our technical decisions and internal project management, if we decide to tackle issues through PRs from our core developers.

I would recommend discussing and pinging us in issues, before beginning to work on them. This way we can better coordinate with you, to ensure excess work is not done.

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.

Remove VueUse dependency if it's not necessary
3 participants