-
Notifications
You must be signed in to change notification settings - Fork 1k
fix[angular-gen2]: Better error handling on form submissions #4031
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
Changes from all commits
8cbb4ea
d0ea341
07dc220
01cb064
8620a68
92a354e
cf36873
7e08c64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
--- | ||
'@builder.io/sdk-angular': patch | ||
'@builder.io/sdk-react-nextjs': patch | ||
'@builder.io/sdk-qwik': patch | ||
'@builder.io/sdk-react': patch | ||
'@builder.io/sdk-react-native': patch | ||
'@builder.io/sdk-solid': patch | ||
'@builder.io/sdk-svelte': patch | ||
'@builder.io/sdk-vue': patch | ||
--- | ||
|
||
Fix: Error handling on form submission |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import { expect } from '@playwright/test'; | ||
import { excludeTestFor, test } from '../helpers/index.js'; | ||
import { excludeGen1, excludeTestFor, test } from '../helpers/index.js'; | ||
|
||
test.describe('Form', () => { | ||
test('Form rendering correctly', async ({ page, sdk }) => { | ||
|
@@ -46,4 +46,52 @@ test.describe('Form', () => { | |
await expect(form.locator('select')).toHaveAttribute('required'); | ||
await expect(form.locator('textarea')).toHaveAttribute('required'); | ||
}); | ||
|
||
test('form submission', async ({ page, sdk }) => { | ||
test.skip( | ||
excludeTestFor({ reactNative: true, rsc: true }, sdk), | ||
'Form not implemented in React Native and NextJS SDKs.' | ||
); | ||
|
||
test.skip(excludeGen1(sdk)); | ||
|
||
// Create console listener before clicking submit | ||
const consoleMessages: string[] = []; | ||
page.on('console', async msg => { | ||
// Get all args and convert them to strings | ||
const args = await Promise.all(msg.args().map(arg => arg.jsonValue())); | ||
const messageText = args.join(' '); | ||
consoleMessages.push(messageText); | ||
}); | ||
|
||
await page.goto('/form'); | ||
|
||
const form = page.locator('form'); | ||
await expect(form).toHaveCount(1); | ||
|
||
const inputs = await form.locator('input').all(); | ||
await inputs[0].fill('Test Name'); | ||
await inputs[1].fill('[email protected]'); | ||
await page.locator('select').selectOption('20-30'); | ||
await page.locator('textarea').fill('Test message'); | ||
|
||
// Submit the form | ||
await form.locator('button').click(); | ||
|
||
// Wait for any console messages to be processed | ||
await page.waitForTimeout(100); | ||
|
||
// Verify error message was logged | ||
if (sdk === 'solid') { | ||
await expect(page.locator('.builder-text').nth(3)).toHaveText( | ||
'SubmissionsToEmail is required when sendSubmissionsTo is set to email' | ||
); | ||
} else { | ||
expect( | ||
consoleMessages.some(msg => | ||
msg.includes('SubmissionsToEmail is required when sendSubmissionsTo is set to email') | ||
) | ||
).toBeTruthy(); | ||
} | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -187,6 +187,21 @@ export default function FormComponent(props: FormProps) { | |||||||||
|
||||||||||
state.formState = 'sending'; | ||||||||||
|
||||||||||
if ( | ||||||||||
props.sendSubmissionsTo === 'email' && | ||||||||||
(props.sendSubmissionsToEmail === '[email protected]' || | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
we can create a function which can validate input emails There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yash-builder This is actually not the email which comes from the form input. This is the email that users set when creating the form. Since this code is not repeating, I'll let it be |
||||||||||
!props.sendSubmissionsToEmail) | ||||||||||
) { | ||||||||||
const message = | ||||||||||
'SubmissionsToEmail is required when sendSubmissionsTo is set to email'; | ||||||||||
console.error(message); | ||||||||||
state.formState = 'error'; | ||||||||||
state.mergeNewRootState({ | ||||||||||
formErrorMessage: message, | ||||||||||
}); | ||||||||||
return; | ||||||||||
} | ||||||||||
|
||||||||||
const formUrl = `${ | ||||||||||
getEnv() === 'dev' ? 'http://localhost:5000' : 'https://builder.io' | ||||||||||
}/api/v1/form-submit?apiKey=${props.builderContext.value.apiKey}&to=${btoa( | ||||||||||
|
@@ -214,21 +229,37 @@ export default function FormComponent(props: FormProps) { | |||||||||
body = await res.text(); | ||||||||||
} | ||||||||||
|
||||||||||
if (!res.ok && props.errorMessagePath) { | ||||||||||
/* TODO: allow supplying an error formatter function */ | ||||||||||
let message = get(body, props.errorMessagePath); | ||||||||||
if (!res.ok) { | ||||||||||
const submitErrorEvent = new CustomEvent('submit:error', { | ||||||||||
detail: { | ||||||||||
error: body, | ||||||||||
status: res.status, | ||||||||||
}, | ||||||||||
}); | ||||||||||
|
||||||||||
if (message) { | ||||||||||
if (typeof message !== 'string') { | ||||||||||
/* TODO: ideally convert json to yaml so it woul dbe like | ||||||||||
error: - email has been taken */ | ||||||||||
message = JSON.stringify(message); | ||||||||||
if (formRef?.nativeElement) { | ||||||||||
formRef?.nativeElement.dispatchEvent(submitErrorEvent); | ||||||||||
if (submitErrorEvent.defaultPrevented) { | ||||||||||
return; | ||||||||||
} | ||||||||||
state.formErrorMessage = message; | ||||||||||
state.mergeNewRootState({ | ||||||||||
formErrorMessage: message, | ||||||||||
}); | ||||||||||
} | ||||||||||
state.responseData = body; | ||||||||||
state.formState = 'error'; | ||||||||||
|
||||||||||
let message = props.errorMessagePath | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we create a small function to get the message? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is not repeated anywhere, why do we need a function. |
||||||||||
? get(body, props.errorMessagePath) | ||||||||||
: body.message || body.error || body; | ||||||||||
|
||||||||||
if (typeof message !== 'string') { | ||||||||||
message = JSON.stringify(message); | ||||||||||
} | ||||||||||
|
||||||||||
state.formErrorMessage = message; | ||||||||||
state.mergeNewRootState({ | ||||||||||
formErrorMessage: message, | ||||||||||
}); | ||||||||||
|
||||||||||
return; | ||||||||||
} | ||||||||||
|
||||||||||
state.responseData = body; | ||||||||||
|
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.
instead of the current
test.skip()
tryThere 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.
is your change only for angular?
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.
@clyde-builderio this change is to address issue in angular SDK. But I think the
form.lite.tsx
is common for all sdks and I want it to work everywhere.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.
Then instead of
test.skip(sdk !== 'angular');
we can do
this will make sure the Gen 1 React SDK tests get skipped
I'll check if there is way to make sure the other Gen 2 tests do not fail
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 I run

yarn g:nx serve @e2e/solid
and then hithttp://localhost:4173/form
in the browser i dont get anyconsole.log()
but I get abuilder-text
below the buttonThere 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.
Same outcome happens if i run

yarn g:nx serve @e2e/solid-start
and hithttp://localhost:3000/form
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.
For the qwik-city failure....looks like a flaky test. Maybe re-running the GitHub check should resolve it
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.
@clyde-builderio Thanks. Thats right for solid sdks somehow the error doesn't get printed on console but shows up on screen. I have updated the test to handle that separately for the solid SDK