Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

angular.copy does not support MediaStream object #16055

Closed
1 of 3 tasks
Narretz opened this issue Jun 15, 2017 · 5 comments
Closed
1 of 3 tasks

angular.copy does not support MediaStream object #16055

Narretz opened this issue Jun 15, 2017 · 5 comments

Comments

@Narretz
Copy link
Contributor

Narretz commented Jun 15, 2017

I'm submitting a ...

  • bug report
  • feature request
  • other (Please do not submit support requests here (see above))

Current behavior:

angular.copy does not work with (Local)MediaStream objects. This affects the watchability of objects that contain such an object.

Expected / new behavior:

Minimal reproduction of the problem with instructions:
https://plnkr.co/edit/B3LebuAQPoj6kRgS2eCG?p=preview

Angular version: 1.x.y

Browser: [all | Chrome XX | Firefox XX | Edge XX | IE XX | Safari XX | Mobile Chrome XX | Android X.X Web Browser | iOS XX Safari | iOS XX UIWebView | iOS XX WKWebView ]

Anything else:

As always with these things, it's debatable if this needs to be fixed (this is probably not the last built.-in that can't be copied). Lodash's clone implementation works, but is also pretty extensive. Maybe we could take a hint from there.

@Narretz Narretz added this to the Ice Box milestone Jun 15, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jun 17, 2017
use MediaStream clone method to clone mediastream object

Closes angular#16055
@m-amr
Copy link
Contributor

m-amr commented Jun 17, 2017

@Narretz

Based on angular.copy implementation it should make a deep copy of nested objects
but _.clone, and _.assignIn create a shallow clone and any nested objects or arrays will be copied by reference, not duplicated.http://underscorejs.org/#clone
That's why copying MediaStream object works they didn't handle it they just return a reference of it.

we can implement a behavior like Lodash and Underscore by just return a reference in copyType method

function copyType(source) {
    switch (toString.call(source)) {
     // code for other cases
      case '[object MediaStream]':
            return source;
    }

Or solving the problem by using MediaStream.clone but it will have a different behavior than Underscore and Lodash because The clone method of the MediaStream interface creates a duplicate of the MediaStream. This new MediaStream object has a new unique id and contains clones of every MediaStreamTrack contained by the MediaStream on which clone() was called.
https://developer.mozilla.org/en-US/docs/Web/API/MediaStream/clone

That means that the copied MediaStream and MediaStreamTrack will have different IDs than than source.

I think this the correct behavior is using `MediaStream.clone'

function copyType(source) {
    switch (toString.call(source)) {
     // code for other cases
      case '[object MediaStream]':
            return source.clone();
    }

I think using the MediaStream.clone will have the correct behavior.

m-amr added a commit to m-amr/angular.js that referenced this issue Jun 17, 2017
use MediaStream clone method to clone mediastream object

Closes angular#16055
m-amr added a commit to m-amr/angular.js that referenced this issue Jun 17, 2017
use MediaStream clone method to clone mediastream object

Closes angular#16055
m-amr added a commit to m-amr/angular.js that referenced this issue Jun 17, 2017
use MediaStream clone method to clone mediastream object

Closes angular#16055
@gkalpak
Copy link
Member

gkalpak commented Jul 3, 2017

The problem with copying special objects, such as MediaStream, is that .copy() can be used for two purposes:

  1. Create a clone/copy of the original object with the expectation that it will have the same functional characteristics as the orignal, but modifying it would not affect the original.
  2. Create a clone/copy of the original object with the expectation that comparing the two (e.g. using .equals() will return true).

(1) is more likely to be used by developers in secases where .copy() is treated as a general purpose cloning utility (which as stated many times is outside the purpose of the function).
(2) is what .copy() is mostly used for internally.

Unfortunately, for (2) using MediaStream.clone() does not allow .equals() to determine whether two MediaStream clones are equal. In that sense, using .copy() on MediaStream objects is a dead-end (for the framework's purposes).

Therefore, I believe it would be better to accept and document that certain objects will not work correctly with .copy() (and thus for deep-watching), instead of trying to keep a promise we actually can't.

@m-amr
Copy link
Contributor

m-amr commented Jul 3, 2017

So you suggest to return a reference of MediaStream object while copying and mention in the documentation that special object like MediaStream will be a shallow copy ?

we can implement a behavior like Lodash and Underscore by just return a reference in copyType method

function copyType(source) {
    switch (toString.call(source)) {
     // code for other cases
      case '[object MediaStream]':
            return source;
    }

Or you mean leave the old behavior and mention in the documentation that copying MediaStream object is not supported ?

@gkalpak
Copy link
Member

gkalpak commented Jul 3, 2017

I am fine with both of these options. Let's see what @Narretz thinks.

@Narretz
Copy link
Contributor Author

Narretz commented Jul 4, 2017

I don't think we return references / do shallow copying for any other type in copy, right? So I'd rather we document that copying MediaStream elements is not supported

@jbedard jbedard self-assigned this May 3, 2018
Narretz added a commit that referenced this issue Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants