-
Notifications
You must be signed in to change notification settings - Fork 89
feat: Adds signInFeature type and validation for form fields in EmailPassword Recipe #976
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
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for precious-marshmallow-968a81 canceled.
|
✅ Deploy Preview for astounding-pegasus-21c111 canceled.
|
validate: field.id === FORM_FIELD_EMAIL_ID ? field.validate : defaultValidator, | ||
optional: false, | ||
}; | ||
function normaliseSignInFormFields(formFields?: TypeInputFormField[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If done this way, it'd be a breaking change: if someone added an override for their email validators in the sign up config, they'd expect it to be applied here. I'd prefer if we kept it that way as well.
// no email field give by user | ||
normalisedFormFields.push({ | ||
id: FORM_FIELD_EMAIL_ID, | ||
validate: defaultEmailValidator, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fall back to the (normalized) email field validator for sign-up to avoid breaking apps.
Hi, thanks for your contribution. I think we should be able to get this merged and released next week :) |
Co-authored-by: Mihály Lengyel <[email protected]>
✅ Deploy Preview for astounding-pegasus-21c111 canceled.
|
✅ Deploy Preview for precious-marshmallow-968a81 canceled.
|
Co-authored-by: Mihály Lengyel <[email protected]>
Minor version instead of patch version
Optimize array validation Co-authored-by: Mihály Lengyel <[email protected]>
Co-authored-by: Mihály Lengyel <[email protected]>
…mized-at-recipe-initialization
}); | ||
} | ||
if (!normalisedFormFields.some((field) => field.id === FORM_FIELD_PASSWORD_ID)) { | ||
// no password field give by user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "no password field give by user" contains a grammatical error. It should be "no password field given by user" to maintain proper grammar. This same typo appears in another comment about email fields as well. Consider correcting both instances for consistency.
// no password field give by user | |
// no password field given by user |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
@@ -12,7 +12,8 @@ | |||
* License for the specific language governing permissions and limitations | |||
* under the License. | |||
*/ | |||
export const version = "22.1.0"; | |||
export const version = "21.2.0"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version number appears to be downgraded from 22.1.0
to 21.2.0
, which is unexpected for a feature addition. This is likely an error in version management. Typically, version numbers should only increase when adding new features, following semantic versioning principles.
export const version = "21.2.0"; | |
export const version = "22.2.0"; |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Summary of change
Introduce a new
signInFeature
type with validation for form fields, ensuring proper handling of email and password fields. Update related functions to accommodate the new feature. Why? Because my use case needs to send additional data to the SignIn POST endpoint and that made things complicated for the only way to achieve this was to override the SignInPost.Related issues
Test Plan
npm run test. All tests have passed.
Documentation changes
Checklist for important updates
coreDriverInterfaceSupported.json
file has been updated (if needed)lib/ts/version.ts
frontendDriverInterfaceSupported.json
file has been updated (if needed)package.json
package-lock.json
lib/ts/version.ts
npm run build-pretty
recipe/thirdparty/providers/configUtils.ts
file,createProvider
function.git tag
) in the formatvX.Y.Z
, and then find the latest branch (git branch --all
) whoseX.Y
is greater than the latest released tag.add-ts-no-check.js
file to include thatsomeFunc: function () {..}
).exports
inpackage.json
Remaining TODOs for this PR