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

Add support for json/text submissions #6570

Merged
merged 5 commits into from Jun 21, 2023

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Jun 8, 2023

Make required changes and add tests for the Remix portion of remix-run/react-router#10413 which adds support for application/json and text/plain submissions

Note: CI will fail until we can point this to a pre/experimental release of RR

Closes #4342

@changeset-bot
Copy link

changeset-bot bot commented Jun 8, 2023

🦋 Changeset detected

Latest commit: df081e8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 18 packages
Name Type
@remix-run/react Minor
@remix-run/testing Minor
create-remix Minor
remix Minor
@remix-run/architect Minor
@remix-run/cloudflare Minor
@remix-run/cloudflare-pages Minor
@remix-run/cloudflare-workers Minor
@remix-run/css-bundle Minor
@remix-run/deno Minor
@remix-run/dev Minor
@remix-run/eslint-config Minor
@remix-run/express Minor
@remix-run/netlify Minor
@remix-run/node Minor
@remix-run/serve Minor
@remix-run/server-runtime Minor
@remix-run/vercel Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines +46 to +59
if (contentType && /\bapplication\/json\b/.test(contentType)) {
init.headers = { "Content-Type": contentType };
init.body = JSON.stringify(await request.json());
} else if (contentType && /\btext\/plain\b/.test(contentType)) {
init.headers = { "Content-Type": contentType };
init.body = await request.text();
} else if (
contentType &&
/\bapplication\/x-www-form-urlencoded\b/.test(contentType)
) {
init.body = new URLSearchParams(await request.text());
} else {
init.body = await request.formData();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensure we proxy the RR request over the wire to Remix with the correct content type and body

@@ -1573,22 +1587,29 @@ function convertRouterFetcherToRemixFetcher(
formMethod &&
formAction &&
formEncType &&
formData
(formData || json || text)
Copy link
Member

Choose a reason for hiding this comment

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

do we want both json !== undefined && text != null here? This would block null and empty string JSON submission, and empty string text submissions. If the "json" value here is pre-encoded (a json string) that one is fine, but the text case I think would cause issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, this also uncovered a small bug in submitting null/"" in RR that I fixed in remix-run/react-router#10625

@@ -63,7 +63,7 @@
"@octokit/graphql": "^4.8.0",
"@octokit/plugin-paginate-rest": "^2.17.0",
"@octokit/rest": "^18.12.0",
"@playwright/test": "^1.28.1",
"@playwright/test": "^1.35.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to latest playwright so headless chromium works properly for remix-run/react-router#9865. The 1.28 version was giving a false positive on the FormData check - so it was throwing the exception (indicating it supported the submitter) but then new FormData(form, submitter) wouldn't include the value. It works fine in our browser testing though so we're OK chalking it up to a headless issue for now and can look for more robust solutions if folks run into issues.

}) => {
test.fail(
Boolean(javaScriptEnabled),
"<Form> doesn't serialize submit buttons correctly #4342"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does now!

@brophdawg11 brophdawg11 merged commit 3cb6e37 into dev Jun 21, 2023
9 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/json-text-serialization branch June 21, 2023 17:07
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Jun 21, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.18.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.18.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label Jun 26, 2023
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.

None yet

2 participants