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

Emit submission payload(s) to host app #291

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eyelidlessness
Copy link
Member

Supersedes and closes #289. Closes #23. Closes #290.

I have verified this PR works in these browsers (latest versions):

  • Chrome
  • Firefox
  • Safari (macOS)
  • Safari (iOS)
  • Not applicable

It would shock me if there's anything browser-specific here.

What else has been done to verify that this works as intended?

Types do much of the work. Added console logs in FormPreview.vue demonstrate both handlers working.

Why is this the best possible solution? Were any other approaches considered?

I mentioned this in Slack, and will reiterate here: in hindsight, since there are two distinct SubmissionResult payload formats, events may not be the best API for this. On the other hand, it may be possible to improve the API by:

  • Making submissionMaxSize generic at the component/package boundary
  • Restoring the single submit event emit, implicitly branching the payload type so it's chunked if submissionMaxSize is non-nullish, monolithic otherwise

This change already felt big enough (especially compared to the #289 1-liner), so I didn't want to hold it up to explore that. Especially because I'm already kind of uncomfortable with the implicit coupling between a prop and emit, and I'm not sure branching on the prop would ameliorate that!

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

I can't think of anything beyond... this exposes existing functionality to a host application, along with any known or unknown bugs that may already be present.

Do we need any specific form for testing your changes? If so, please attach one.

N/A

What's changed

The main @getodk/web-forms component now:

  • Emits a MonolithicSubmissionResult if a host application attaches a submit event handler
  • Adds an optional submissionMaxSize prop
  • Emits a ChunkedSubmissionResult if a host application specifies submissionMaxSize and attaches a submitChunked event handler

I’m not really sure I understand why the simpler signature was wrong! It’s also possible the inference behavior is different in Vue (where I observed wrong-ness).
…ed` events

Provides host application access to submission payload data, in a relatively coherent component API.
This isn’t a great sign about the engine-side change. I’m going ahead with it to get this into review quickly, unblocking both the Central integration and my ability to get back to reviewing other work in flight.

I think it’s okay to just remove this parameter for now, because there won’t be tests exercising chunked submissions until we support `<upload>` and submission attachments.
Copy link

changeset-bot bot commented Jan 24, 2025

🦋 Changeset detected

Latest commit: c60e37a

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

This PR includes changesets to release 2 packages
Name Type
@getodk/web-forms Minor
@getodk/xforms-engine Patch

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

Copy link
Collaborator

@latin-panda latin-panda left a comment

Choose a reason for hiding this comment

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

It looks good to me! Just one question below but other than that, it's approved.

packages/web-forms/src/demo/FormPreview.vue Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants