Shopify / shopify-app-bridge

https://shopify.dev/docs/api/app-bridge
82 stars 9 forks source link

Contextual Save Bar 'Discard' crashes with 'Illegal Invocation' on textarea #230

Closed rikbrown closed 10 months ago

rikbrown commented 10 months ago

Describe the bug

The contextual save bar's Discard button crashes if there's a text area.

To Reproduce

Create a form with a text area using the contextual save bar, modify it, then try to discard.

image image

This appears (based on my debugging only) to be because it's trying to use this code to reset it: const c = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, "value");.

Expected behaviour

Textarea is reset.

Contextual information

Packages and versions

List the relevant packages you’re using, and their versions. For example:

"@shopify/app": "3.48.3",
"@shopify/app-bridge-types": "^0.0.2",
"@shopify/cli": "3.48.3",
"@shopify/polaris": "12.0.0-beta.0",
"@shopify/polaris-icons": "^7.8.1",
"@shopify/shopify-app-remix": "^1.0.3",
"@shopify/shopify-app-session-storage-prisma": "^1.0.0",

app bridge via cdn

Platform

Remix/Chromium

Additional context

rikbrown commented 10 months ago

General meta point: we should have some control over what this button does for more complex form experiences.

rikbrown commented 10 months ago

Actually older versions seemed to support that. Maybe I'll go back to those.

developit commented 10 months ago

Sorry about that! I just deployed a fix that removes the crash.

FWIW, I believe we'll be adding direct save bar control at some point, but for now if you need explicit control over it you should be able to use a hidden form:

function SaveBar({ dirty = false, loading = false, onSave, onReset }) {
  const ref = useRef();
  useEffect(() => {
    ref.current.value = +dirty;
  }, [dirty]);
  return (
    <form data-save-bar={loading?'loading':''} onSubmit={onSave} onReset={onReset}>
      <input initialValue="0" ref={ref} />
    </form>
  );
}

I'd be interested to understand where you're thinking you'd want explicit control though. The save bar is designed to mimic the behavior of the browser's existing dirty tracking and <button type=submit>/<button type=reset> functionality, so it ideally shouldn't be something you have to think about (aside from this bug).

rikbrown commented 6 months ago

Sorry for the late reply @developit - and thanks for fixing that.

Any update on the direct save control?

Your approach works though (with some questions - see below). To answer your question of why I need to do this: I have some complex form data maintained using React Hook Form. In particular, there's some data (a JSON blob) which I use to render components (which in turn modify that data)... but they're not traditional form elements. I suppose I could put the data into a hidden input or something like that as an interim, but that's effectively what your hack does.

Back to some of my questions:

  1. In your example you set data-save-bar="loading"... this isn't documented but I'm guessing it's a way to trigger the "Save" button to show a loading state. This doesn't seem to work though. Is this an upcoming feature?
  2. I'm finding the data-save-bar stuff doesn't handle saves/resets very well: I'm saving using an async post (to my Remix app's action). So once the save happens the form is no longer dirty. I'm not sure that the data-save-bar stuff is able to be aware of that... subsequent updates don't seem to re-trigger the save bar to appear. Is there some way to handle this?

All in all I like the idea of having this as a built-in part of the form... but the "magic" doesn't seem to be quite there compared to the older version where you just explicitly told the bar to appear.

rikbrown commented 6 months ago

I ended up with this to make it seemingly work:

function SaveBar({
  dirty = false,
  loading = false,
  onSave,
  onReset,
}: {
  dirty: boolean
  loading: boolean
  onSave: FormEventHandler
  onReset: () => void
}) {
  const ref = useRef<HTMLInputElement>(null)
  useEffect(() => {
    if (!ref.current) return
    if (dirty) {
      ref.current.value = `${Math.random()}` // seemed to be necessary, otherwise subsequent clean->dirty transitions did not trigger the save bar
      ref.current.dispatchEvent(new Event("change", { bubbles: true })) // wasn't triggering at all unless I faked a change event
    } else {
      ref.current.value = "0"
    }
  }, [dirty])

  return (
    <form data-save-bar={loading ? "loading" : ""} onSubmit={onSave} onReset={onReset}>
      <input value="0" ref={ref} />
    </form>
  )
}

This has one bug - hitting enter on a field to save it seems to keep the save-bar there, I haven't figured out why. Maybe it's getting a double change event in that case.

This definitely feels pretty hacky at this point so I'm going to see if I can just get it working more traditionally as well.