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: add a feature flag for new field validation behavior #14285

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

vursen
Copy link
Contributor

@vursen vursen commented Aug 8, 2022

Description

This PR adds a feature flag for the new field validation behavior. The flag will be used by Flow field components. For more information about the new behavior, please refer to vaadin/platform#3066.

Part of vaadin/platform#3066

Type of change

  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Copy link
Contributor

@mshabarov mshabarov left a comment

Choose a reason for hiding this comment

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

Conflicts with master should be resolved.

By the way, where is this flag supposed to be checked? Does the Flow's Binder need to check it and behave accordingly?

@github-actions
Copy link

github-actions bot commented Aug 8, 2022

Unit Test Results

   916 files  +  9     916 suites  +9   51m 9s ⏱️ + 4m 19s
6 010 tests +11  5 956 ✔️ +11  54 💤 ±0  0 ±0 
6 228 runs  +  5  6 165 ✔️ +  5  63 💤 ±0  0 ±0 

Results for commit 54be8f1. ± Comparison against base commit 1bf5831.

♻️ This comment has been updated with latest results.

@vursen
Copy link
Contributor Author

vursen commented Aug 8, 2022

By the way, where is this flag supposed to be checked? Does the Flow's Binder need to check it and behave accordingly?

The flag is planned to be used only in Flow field components.

mshabarov
mshabarov previously approved these changes Aug 8, 2022
@vursen
Copy link
Contributor Author

vursen commented Aug 8, 2022

Here is an alternative naming for the flag suggested by @yuriy-fix: synchronizedValidation

@knoobie
Copy link
Contributor

knoobie commented Aug 8, 2022

Another name could be (enforce) Field / Client Validation because it enables the field validation to run before the binder. I don't think a lot of people understand the concept of chaining and validation ordering.

@vursen
Copy link
Contributor Author

vursen commented Aug 8, 2022

@knoobie Good point, I agree that the current naming is rather not descriptive and that we need to find a better alternative.

Just want to point out that the field constraint validation has already been somewhat integrated into the binder validation through implementing getDefaultValidator and this is already available without feature flags, so the field validation is already enforced in a way. It is now more about enforcing client validation...

That being said, the above doesn't change things much and your suggested naming would still work, I think. I wonder how we should name the feature flag itself then: enforceClientFieldValidation? enforceClientValidation?

@knoobie
Copy link
Contributor

knoobie commented Aug 8, 2022

and this is already available without feature flags, so the field validation is already enforced in a way.

Oh! I was expecting the whole epic (including the field changes) would be behind the feature flag and not just the client side. (Don't mind, was just unexpected)

enforceClientFieldValidation

I would think this describes it the best

@vursen
Copy link
Contributor Author

vursen commented Aug 9, 2022

As a result of an internal discussion, we came to the agreement that

  • We will hide everything related to the new validation behavior behind a feature flag, including the recent getDefaultValidator changes
  • We will name the feature flag enforceFieldValidation

@vursen vursen force-pushed the feat/add-validation-feature-flag branch from 2f17a08 to 69d4789 Compare August 9, 2022 08:57
@vursen vursen changed the title feat: add a feature flag for chained validation feat: add a feature flag for the new validation behavior Aug 9, 2022
@vursen vursen changed the title feat: add a feature flag for the new validation behavior feat: add a feature flag for new field validation behavior Aug 9, 2022
@vursen vursen requested a review from mshabarov August 9, 2022 09:00
@mshabarov mshabarov enabled auto-merge (squash) August 9, 2022 12:03
@sonarcloud
Copy link

sonarcloud bot commented Aug 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 23.2.0.beta1 and is also targeting the upcoming stable 23.2.0 version.

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

Successfully merging this pull request may close these issues.

5 participants