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

can't use `:srcObject.prop` to set prop to null #9138

Open
freddialpad opened this Issue Dec 3, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@freddialpad

freddialpad commented Dec 3, 2018

Version

2.5.17

Reproduction link

https://codesandbox.io/s/ppyq71wrxj

Steps to reproduce

(See minimal repro)

  1. have a component with an HTMLMediaElement that binds :srcObject.prop to some data property (that starts off as null):
<audio :srcObject.prop="stream">
  1. set it to some valid MediaStream:
this.stream = new MediaStream();
  1. set it back to null:
this.stream = null;

What is expected?

I expect the audio.srcObject to be null

What is actually happening?

it stays as the MediaStream and there's a TypeError in the log.


I noticed that it did actually work in my first attempt to write the repro example: https://jsfiddle.net/4tnkapxo/1/ (probably because the whole thing is inline?)

Im not sure how helpful this is, but based on the stack trace it's related to this code, found in updateDOMProps:

  for (key in oldProps) {
    if (isUndef(props[key])) {
      elm[key] = '';
    }
  }

isUndef(null) is evaluating true, and so it's trying to set the prop to '', but srcObject specifically needs it to be null or a MediaStream

@posva posva added the improvement label Dec 4, 2018

@posva

This comment has been minimized.

Member

posva commented Dec 4, 2018

maybe we shouldn't apply transformation for .prop bindings

@freddialpad

This comment has been minimized.

freddialpad commented Dec 13, 2018

Now that I've looked at the code (updateDOMProps in src/platforms/web/runtime/modules/dom-props.js) in question I think I understand a little better what's happening. The associated test case (in unit/modules/vdom/modules/dom-props.spec.js) makes it seem like it's guarding against deletion:

  it('should remove the elements domProps', () => {
    const vnode1 = new VNode('a', { domProps: { src: 'http://localhost/' }})
    const vnode2 = new VNode('a', { domProps: {}})
    patch(null, vnode1)
    const elm = patch(vnode1, vnode2)
    expect(elm.src).toBe('')
  })

...There's no generic way to "delete" an HTMLEmelent property aside from setting it to whatever arbitrary value the setter defines. In that sense, casting to empty string as elm[key] = ''; Is basically the closest there is to a standard way. in the case of an <img> tag, setting the .src prop to anything beside empty string results in some pretty obnoxious behavior:

09:48:05.267 const img = document.createElement('img')
09:48:08.948 img.src
09:48:08.953 ""
09:48:14.469 img.src = null
09:48:17.213 img.src
09:48:17.222 "https://localhost:8080/null"

(I'm sure there's a great reason for that behavior and it has legitimate use cases all over.)

So in that context, empty string seems like as reasonable a default as any for null/undef props. I've experimented with changing the isUndef(props[key]) check to props.hasOwnProperty(key), or even just comparing it directly to undefined... pretty simple to do, but it doesn't really seem viable if users are depending on the ability to delete a prop by setting it to null; it's not backwards compatible. And special-casing property/element combinations in dom-props.js seems too obviously terrible to even mention.

That said, am I making too many assumptions there? is that code purely a guard against missing props in the new vnode? was the author of the test only considering src property because it's what caused a failure? is it reasonable to expect that users supply the correct value from among "", null and undefined when "deleting" a prop?

On the other hand, I guess it doesn't really make sense to talk about deletion when it comes to bound properties... like in the example of <audio :srcObject.prop="stream"> the stream computed property will have to return something, so I'm assuming there's something with the vnode internals that are beyond my understanding that makes it possible for {stream: {some: thing}} to be replaced with {}.

If the above suggestion sounds okay, then I would assume hasOwnProperty is the right check to make there, and the result should be elm[key] = '' (like I said it's as good a "standard" delete as any, and probably the most commonly applicable case); I'll probably write a pull request this week. But I don't want to spend the time if it's not a safe change to make.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment