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

feat(Form): update and migrate valibot to v0.31.0 #1848

Merged
merged 7 commits into from
Jun 11, 2024

Conversation

fabian-hiller
Copy link
Contributor

@fabian-hiller fabian-hiller commented Jun 8, 2024

Resolves #1849

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ 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

I am the author of Valibot and have updated and migrated Nuxt UI to our latest version: https://valibot.dev/blog/valibot-v0.31.0-is-finally-available/

Valibot is still v0. Breaking changes are therefore expected and the update should be no problem.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@benjamincanac benjamincanac changed the title feat(valibot): update and migrate valibot to v0.31.0 feat(Form): update and migrate valibot to v0.31.0 Jun 10, 2024
@benjamincanac
Copy link
Member

Thanks a lot @fabian-hiller for taking the time to help us with this and for creating this awesome library! 😊

@noook
Copy link
Collaborator

noook commented Jun 10, 2024

Does it also resolve #625 ?

package.json Outdated
@@ -54,6 +54,7 @@
"nuxt-icon": "^0.6.10",
"ohash": "^1.1.3",
"pathe": "^1.1.2",
"valibot": "^0.31.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Valibot should be an opt-in dependency to promote flexibility in validation library selection and minimize dependencies. It should stay as a dev dependency.

}))
}
return []
const result = await valibotParse(schema, state)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to include the parse function inside the schema object so we can fix issues related to the use of _parse without a direct dependency to valibot?

@fabian-hiller
Copy link
Contributor Author

When Valibot was first added, a dynamic import was used to use safeParseAsync. Later on safeParseAsync was replaced by a direct call of the ._parse method. https://github.com/nuxt/ui/pull/617/files

Back then this was ok because safeParseAsync just called ._parse and did nothing else, but this has changed. That's why we need to use safeParseAsync again, but I don't recommend a dynamic import because I don't think it's tree shakable and it breaks the idea behind Valibot.

We now have two alternatives, the first is to add Valibot as a dependency to Nuxt UI to be able to import safeParseAsync (about 200 bytes). The other would be to force your users to wrap there Valibot schema in our new safeParser or safeParserAsync method. This function returns a callable function that internally calls safeParse or safeParseAsync.

const schema = v.object({
  email: v.string(),
  password: v.string()
})

<UForm :schema="v.safeParser(schema)" ... />

@danielroe recommended via Discord to just add Valibot as a dependency because it makes it easier to use, but in the end both solutions work.

Copy link
Member

I understand your point but the Form component is meant to be used with either valibot, yup, zod or joi so it seems wrong to me to add valibot as a direct dependency as it would imply doing the same thing for the other validation libraries πŸ€”

@romhml
Copy link
Collaborator

romhml commented Jun 10, 2024

Thanks for the explanation! I think the using the v.safeParser(schema) would be better for us here. Although this means we need to keep _parse as a fallback for valibot validation to avoid introducing a breaking change.

@Gerbuuun
Copy link

Gerbuuun commented Jun 10, 2024

In the PR I closed in favor of this one, I made the change like this.
SchermΒ­afbeelding 2024-06-10 om 12 51 32
This is almost the same as how v.safeParse() does it internally. But this way you do not have valibot as a dependency?

edit: seems like this way the global config will not be used...

@fabian-hiller
Copy link
Contributor Author

edit: seems like this way the global config will not be used...

Yes, that's the problem. i18n doesn't work that way.

Although this means we need to keep _parse as a fallback for valibot validation to avoid introducing a breaking change.

Since Valibot is still v0, a breaking change should be ok for Valibot users, but we can of course support both.

We could also support all 3 options:

  • ._parse(…) for Valibot <0.31.0
  • ._run(…) for Valibot >=0.31.0 without global config but less boilerplate
  • safeParser for Valibot >=0.31.0 with global config

Please let me know what you decide.

@romhml
Copy link
Collaborator

romhml commented Jun 10, 2024

We could also support all 3 options:
._parse(…) for Valibot <0.31.0
._run(…) for Valibot >=0.31.0 without global config but less boilerplate
safeParser for Valibot >=0.31.0 with global config

Adding support for the three options looks like a great compromise. We'll be able to cleanly drop support for versions below 0.31.0 in our next major release.

@fabian-hiller
Copy link
Contributor Author

I have updated the implementation.

Copy link
Collaborator

@romhml romhml left a comment

Choose a reason for hiding this comment

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

Thanks @fabian-hiller πŸ˜„

src/runtime/components/forms/Form.vue Outdated Show resolved Hide resolved
package.json Outdated
@@ -71,7 +71,8 @@
"release-it": "^17.3.0",
"typescript": "^5.4.5",
"unbuild": "^2.0.0",
"valibot": "^0.30.0",
"valibot30": "npm:[email protected]",
"valibot31": "npm:valibot@^0.31.1",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be better for typings to keep the latest version as valibot?

Suggested change
"valibot31": "npm:valibot@^0.31.1",
"valibot": "^0.31.1",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe there is no difference. I just followed a StackOverflow discussion to install two different versions of the same package. If we change the package name, we also need to update the import statement.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah in the Form.vue component we could import from valibot30 and valibot (instead of valibot31).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@benjamincanac benjamincanac merged commit 1d5bd89 into nuxt:dev Jun 11, 2024
2 checks passed
@benjamincanac
Copy link
Member

Thanks @fabian-hiller! 😊

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.

Update form component to support Valibot to v0.31.0
5 participants