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

fix: validation of missing properties in helpers.forEach #1228

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

haug1
Copy link

@haug1 haug1 commented Feb 26, 2024

Summary

Describe what your PR does in a few short words

validations on properties inside helpers.forEach that were missing in the target model were skipped

(I think there are several issues referencing this... I'm just fixing it because I need to deal with it in an app that I'm maintaining. Please get this merged so I can unpin my modified version of vuelidate from my repo.)

Here's a simple reproduction:

<template>
  <div
    v-for="(input, index) in state.collection"
    :key="index"
  >
    <input v-model="input.name" type="text" />
    <div
      v-for="error in v$.collection.$each.$response.$errors[index].name"
      :key="error"
    >
      {{ error.$message }}
    </div>
  </div>
</template>
<script>
import { helpers, required } from "@vuelidate/validators";
import { useVuelidate } from "@vuelidate/core";
import { reactive } from "vue";

export default {
  setup() {
    const state = reactive({
      collection: [{ name: "" }, {}],
    });
    return {
      v$: useVuelidate(
        {
          collection: {
            $each: helpers.forEach({
              name: {
                required,
              },
            }),
          },
        },
        state
      ),
      state,
    };
  },
};
</script>

Running the above code, you will notice that vuelidate incorrectly does not report errors for the second object in the array.

Metadata

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No
  • Maybe?

The validation result does change a tiny bit, see the diff in the test. Properties without validators are now omitted from the validation result instead of appearing empty. I assume this change may possibly break a consumer app with very specific implementations of how they read the validation result from the forEach. I've decided to ignore this possibly breaking change, since it seems inherently counter intuitive that properties without validators appear in the validation result and that is different behavior to how it normally behaves outside of the forEach function. Any implementation that leans on this behavior must be doing it for no particular good reason, only a quirk they've had to accomodate for in the past.

@haug1 haug1 force-pushed the fix/helpers-forEach branch 4 times, most recently from 3dcc22d to 2fcc337 Compare February 26, 2024 15:21
@haug1
Copy link
Author

haug1 commented Feb 26, 2024

I've seen this now: #1050. Why is there not any progress on this clear and obvious error in the library for almost a year? Is there hope of getting this merged? If you're so concerned about the breaking change that the properties without validators return empty validation results now disappear, then there should be ways to work around that and still fix the bug. It would only take little more code to add the empty validation results for those properties. Personally, I think it's better to just remove it because it's a pointless extra operation, but maybe there's some implications or functionality to it that I'm not thinking about..

Update: I now understand that it may break some validation functions, primarily those made specifically for usage inside a helpers.forEach? Because they were maybe implemented not expecting undefined to get passed as a value before? But we are migrating from v0.7.7 and trying to replace the usage with this one and it breaks. The documentation says it's there specifically as a migration path and discourages usage and instead opt for nested components. So I think the helpers.forEach function has never been the replacement it was intended to be.

TL;DR: If this is decided to not be merged, could it maybe be a desirable option to expose a new function with this change implemented as a true migration option from v0.x, in case anyone else may stumble down the same path?

@haug1 haug1 force-pushed the fix/helpers-forEach branch 2 times, most recently from 2fcc337 to 2c6c998 Compare February 26, 2024 19:06
@haug1 haug1 changed the title fix: validation of nested properties in helpers.forEach fix: validation of missing properties in helpers.forEach Feb 26, 2024
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.

None yet

1 participant