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

Support direct handlers in useSubmit/fetcher.submit/fetcher.load #10362

Merged
merged 13 commits into from
Apr 21, 2023

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Apr 17, 2023

Depends on #10342

See the changesets for examples.

Right now, the inferred typings aren't in this PR since we were running into some issues with them and may fork them to a new PR. WIP is in the brophdawg11/direct-action-types branch.

@changeset-bot
Copy link

changeset-bot bot commented Apr 17, 2023

🦋 Changeset detected

Latest commit: b99ed0f

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

This PR includes changesets to release 5 packages
Name Type
@remix-run/router Minor
react-router-dom Minor
react-router Minor
react-router-dom-v5-compat Minor
react-router-native 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

@brophdawg11 brophdawg11 changed the title Support direction actions in submit/fetcher.submit Support direct actions in submit/fetcher.submit Apr 17, 2023
@brophdawg11 brophdawg11 self-assigned this Apr 17, 2023
@brophdawg11 brophdawg11 force-pushed the brophdawg11/direct-action branch 2 times, most recently from a08fba1 to 269e4b0 Compare April 18, 2023 17:47
@brophdawg11 brophdawg11 changed the title Support direct actions in submit/fetcher.submit Support direct handlers in useSubmit/fetcher.submit/fetcher.load Apr 18, 2023
@@ -1,4 +1,4 @@
import type { History, Location, Path, To } from "./history";
import type { Action, History, Location, Path, To } from "./history";
Copy link
Member

Choose a reason for hiding this comment

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

This type isn't used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stupid VSCode auto-imports 🤦

@brophdawg11 brophdawg11 force-pushed the brophdawg11/direct-action branch 2 times, most recently from 7f1f91d to ea5fa1b Compare April 21, 2023 18:53
Base automatically changed from brophdawg11/opt-out-serialization to dev April 21, 2023 18:54
Comment on lines -185 to -187
let submissionTrigger: HTMLButtonElement | HTMLInputElement = (
options as any
).submissionTrigger;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was no longer being used - ever since we started sending from the submitter property on the event

Comment on lines -189 to -191
if (options.action) {
action = options.action;
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For all cases (action/encType/method/etc.), options[field] took precedence, and now that action could be an ActionFunction it made sense to pull that out - so all options[field] overrides happen in the body of useSubmit now instead of in getFormSubmissionInfo. This also better prepares us for pulling useSubmit down into react-router where it can skip the DOM stuff.

Comment on lines +970 to +973
let path =
typeof options.action === "function" ? null : options.action || action;
let routerAction =
typeof options.action === "function" ? options.action : null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

path and action being sent to the router are mutually exclusive

Comment on lines +1080 to +1093
if (
to != null &&
opts &&
"action" in opts &&
typeof opts.action === "function"
) {
to = null;
warning(
false,
"router.navigate() should not include a `to` location when a custom " +
"`action` is passed, the `to` will be ignored in favor of the current " +
"location."
);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryanflorence This feels like maybe the API isn't quite right, but I didn't like navigating to a "function"

// Current API - `to` is ignored when action is passed
// router.navigate(to: To, opts: RouterNavigationOptions)
router.navigate(null, { 
  formMethod: 'post',
  formData: new FormData(),
  action: customAction,
})

// Alternate API - custom action is passed as the `to` value
// router.navigate(to: To | ActionFunction, opts: RouterNavigationOptions)
router.navigate(customAction, {
  formMethod: 'post',
  formData: new FormData(),
});

@brophdawg11 brophdawg11 merged commit 4357e37 into dev Apr 21, 2023
@brophdawg11 brophdawg11 deleted the brophdawg11/direct-action branch April 21, 2023 19:16
brophdawg11 added a commit that referenced this pull request Apr 26, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.11.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!

@brophdawg11
Copy link
Contributor Author

Silly bot, these were reverted from dev so they're not in 6.11.0-pre.0. We plan to get them re-added with the latest changes from the Proposal in 6.12.

brophdawg11 added a commit that referenced this pull request Apr 28, 2023
@remix-run remix-run deleted a comment from github-actions bot Apr 28, 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.

3 participants