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

Enable progressive enhanced form actions through decodeAction #49187

Merged
merged 2 commits into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ const nextAppLoader: AppLoader = async function nextAppLoader() {

export * as serverHooks from 'next/dist/client/components/hooks-server-context'

export { renderToReadableStream, decodeReply } from 'react-server-dom-webpack/server.edge'
export { renderToReadableStream, decodeReply, decodeAction } from 'react-server-dom-webpack/server.edge'
export const __next_app_webpack_require__ = __webpack_require__
export { preloadStyle, preloadFont, preconnect } from 'next/dist/server/app-render/rsc/preloads'

Expand Down
67 changes: 39 additions & 28 deletions packages/next/src/server/app-render/action-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ export async function handleAction({
}): Promise<undefined | RenderResult | 'not-found'> {
let actionId = req.headers[ACTION.toLowerCase()] as string
const contentType = req.headers['content-type']
const isFormAction =
const isURLEncodedAction =
req.method === 'POST' && contentType === 'application/x-www-form-urlencoded'
const isMultipartAction =
req.method === 'POST' && contentType?.startsWith('multipart/form-data')
Expand All @@ -181,7 +181,7 @@ export async function handleAction({
typeof actionId === 'string' &&
req.method === 'POST'

if (isFetchAction || isFormAction || isMultipartAction) {
if (isFetchAction || isURLEncodedAction || isMultipartAction) {
let bound = []

const workerName = 'app' + pathname
Expand Down Expand Up @@ -210,7 +210,7 @@ export async function handleAction({
await actionAsyncStorage.run({ isAction: true }, async () => {
if (process.env.NEXT_RUNTIME === 'edge') {
// Use react-server-dom-webpack/server.edge
const { decodeReply } = ComponentMod
const { decodeReply, decodeAction } = ComponentMod

const webRequest = req as unknown as WebNextRequest
if (!webRequest.body) {
Expand All @@ -220,7 +220,14 @@ export async function handleAction({
if (isMultipartAction) {
// TODO-APP: Add streaming support
const formData = await webRequest.request.formData()
bound = await decodeReply(formData, serverModuleMap)
if (isFetchAction) {
bound = await decodeReply(formData, serverModuleMap)
} else {
const action = await decodeAction(formData, serverModuleMap)
await action()
// Skip the fetch path
return
}
} else {
let actionData = ''

Expand All @@ -234,16 +241,9 @@ export async function handleAction({
actionData += new TextDecoder().decode(value)
}

if (isFormAction) {
if (isURLEncodedAction) {
const formData = formDataFromSearchQueryString(actionData)
actionId = formData.get('$$id') as string

if (!actionId) {
// Return if no action ID is found, it could be a regular POST request
return
}
formData.delete('$$id')
bound = [formData]
bound = await decodeReply(formData, serverModuleMap)
} else {
bound = await decodeReply(actionData, serverModuleMap)
}
Expand All @@ -253,28 +253,41 @@ export async function handleAction({
const {
decodeReply,
decodeReplyFromBusboy,
decodeAction,
} = require(`react-server-dom-webpack/server.node`)

if (isMultipartAction) {
const busboy = require('busboy')
const bb = busboy({ headers: req.headers })
req.pipe(bb)
if (isFetchAction) {
const busboy = require('busboy')
const bb = busboy({ headers: req.headers })
req.pipe(bb)

bound = await decodeReplyFromBusboy(bb, serverModuleMap)
bound = await decodeReplyFromBusboy(bb, serverModuleMap)
} else {
// React doesn't yet publish a busboy version of decodeAction
// so we polyfill the parsing of FormData.
const { Readable } = require('stream')
const UndiciRequest = require('next/dist/compiled/undici').Request
const fakeRequest = new UndiciRequest('http://localhost', {
method: 'POST',
headers: { 'Content-Type': req.headers['content-type'] },
body: Readable.toWeb(req),
duplex: 'half',
})
const formData = await fakeRequest.formData()
const action = await decodeAction(formData, serverModuleMap)
await action()
// Skip the fetch path
return
}
} else {
const { parseBody } =
require('../api-utils/node') as typeof import('../api-utils/node')
const actionData = (await parseBody(req, '1mb')) || ''

if (isFormAction) {
actionId = actionData.$$id as string
if (!actionId) {
// Return if no action ID is found, it could be a regular POST request
return
}
if (isURLEncodedAction) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't ever use URL encoded bodies atm but if we ever did, it'd be equivalent of passing formData.

const formData = formDataFromSearchQueryString(actionData)
formData.delete('$$id')
bound = [formData]
bound = await decodeReply(formData, serverModuleMap)
} else {
bound = await decodeReply(actionData, serverModuleMap)
}
Expand Down Expand Up @@ -302,9 +315,7 @@ export async function handleAction({
}
})

if (actionResult) {
return actionResult
}
return actionResult
} catch (err) {
if (isRedirectError(err)) {
if (process.env.NEXT_RUNTIME === 'edge') {
Expand Down
5 changes: 4 additions & 1 deletion test/e2e/app-dir/actions/app-action.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,10 @@ createNextDescribe(
})

it('should support notFound', async () => {
const browser = await next.browser('/server')
const browser = await next.browser('/server', {
// TODO we should also test this with javascript on but not-found is not implemented yet.
disableJavaScript: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably run all these tests with and without JS.

})

await browser.elementByCss('#nowhere').click()

Expand Down
16 changes: 11 additions & 5 deletions test/e2e/app-dir/actions/app/server/form.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ async function nowhere() {
notFound()
}

async function here() {
'use server'
// nothing
}

async function file(formData) {
'use server'
const file = formData.get('file')
Expand All @@ -25,19 +30,20 @@ export default function Form() {
return (
<>
<hr />
<form method="POST" action="">
<input type="text" name="$$id" value={action.$$id} hidden readOnly />
<form action={action}>
<input type="text" name="name" id="name" required />
<button type="submit" id="submit">
Submit
</button>
</form>
<hr />
<form method="POST" action="">
<input type="text" name="$$id" value={nowhere.$$id} hidden readOnly />
<button type="submit" id="nowhere">
<form>
<button formAction={nowhere} type="submit" id="nowhere">
Go nowhere
</button>
<button formAction={here} type="submit" id="here">
Go here
</button>
</form>
<hr />
<form action={file}>
Expand Down
6 changes: 5 additions & 1 deletion test/lib/browsers/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,11 @@ export class BrowserInterface implements PromiseLike<any> {
})
}

async setup(browserName: string, locale?: string): Promise<void> {}
async setup(
browserName: string,
locale: string,
javaScriptEnabled: boolean
): Promise<void> {}
async close(): Promise<void> {}
async quit(): Promise<void> {}

Expand Down
21 changes: 18 additions & 3 deletions test/lib/browsers/playwright.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import path from 'path'
let page: Page
let browser: Browser
let context: BrowserContext
let contextHasJSEnabled: boolean = true
let pageLogs: Array<{ source: string; message: string }> = []
let websocketFrames: Array<{ payload: string | Buffer }> = []

Expand Down Expand Up @@ -47,8 +48,7 @@ export class Playwright extends BrowserInterface {
this.eventCallbacks[event]?.delete(cb)
}

async setup(browserName: string, locale?: string) {
if (browser) return
async setup(browserName: string, locale: string, javaScriptEnabled: boolean) {
const headless = !!process.env.HEADLESS
let device

Expand All @@ -62,8 +62,23 @@ export class Playwright extends BrowserInterface {
}
}

if (browser) {
if (contextHasJSEnabled !== javaScriptEnabled) {
// If we have switched from having JS enable/disabled we need to recreate the context.
await context?.close()
context = await browser.newContext({
locale,
javaScriptEnabled,
...device,
})
contextHasJSEnabled = javaScriptEnabled
}
return
}

browser = await this.launchBrowser(browserName, { headless })
context = await browser.newContext({ locale, ...device })
context = await browser.newContext({ locale, javaScriptEnabled, ...device })
contextHasJSEnabled = javaScriptEnabled
}

async launchBrowser(browserName: string, launchOptions: Record<string, any>) {
Expand Down
2 changes: 1 addition & 1 deletion test/lib/browsers/selenium.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export class Selenium extends BrowserInterface {
private browserName: string

// TODO: support setting locale
async setup(browserName: string, locale?: string) {
async setup(browserName: string, locale: string, javaScriptEnabled: boolean) {
if (browser) return
this.browserName = browserName

Expand Down
4 changes: 3 additions & 1 deletion test/lib/next-webdriver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export default async function webdriver(
disableCache?: boolean
beforePageLoad?: (page: any) => void
locale?: string
disableJavaScript?: boolean
}
): Promise<BrowserInterface> {
let CurrentInterface: typeof BrowserInterface
Expand All @@ -77,6 +78,7 @@ export default async function webdriver(
disableCache,
beforePageLoad,
locale,
disableJavaScript,
} = options

// we import only the needed interface
Expand All @@ -96,7 +98,7 @@ export default async function webdriver(

const browser = new CurrentInterface()
const browserName = process.env.BROWSER_NAME || 'chrome'
await browser.setup(browserName, locale)
await browser.setup(browserName, locale, !disableJavaScript)
;(global as any).browserName = browserName

const fullUrl = getFullUrl(
Expand Down