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

Don't use unnecessary polymer custom elements in WebRTC samples #1148

Closed
jan-ivar opened this issue Oct 31, 2018 · 11 comments · Fixed by #1329
Closed

Don't use unnecessary polymer custom elements in WebRTC samples #1148

jan-ivar opened this issue Oct 31, 2018 · 11 comments · Fixed by #1329

Comments

@jan-ivar
Copy link
Member

#1144 (review) introduced dependencies on polymer custom element stuff that shouldn't have been introduced in samples IMHO. The samples don't even load on Edge anymore.

Samples should be simple and accessible, without unnecessary dependencies, and these dependencies are clearly unnecessary. I suggest they be reverted.

@fippo
Copy link
Collaborator

fippo commented Oct 31, 2018

Also that PR adds recording which is not supported in Edge (if it was loading).
Additionally we have striven to promote the spec in the past, showing the non-getDisplayMedia way in Firefox would have been a no-go.

As a long time contributor to this repository I am concerned about the shift in values (see also #1147 or here)

@jan-ivar
Copy link
Member Author

showing the non-getDisplayMedia way in Firefox would have been a no-go.

Thanks @fippo I missed that. Yeah getDisplayMedia is already shimmed by adapter.js, so this piece of feature-detection code is truly redundant and should be removed.

Also a heads-up about webrtcHacks/adapter#896.

@dagingaa
Copy link

As a web developer and occasional bug reporter I can't tell you how useful it has been to be able to point to WebRTC samples in the past in bug reports or when asking for feature support to link to the sample showing a reference implementation without any cruft or stuff. It has also been a good way to test if the reference implementation has the same bug as I observe in my own application, to get a check for if it's my problem or not.

Moving to polymer feels like the wrong move as it has no place in WebRTC-land, and given that Edge breaks with it, we remove the one reference implementation that strived to work across browsers.

Please keep app frameworks out of the WebRTC samples (or at least use one of the more popular ones that has broad cross-browser support if you must)

@ErikHellman
Copy link
Contributor

Good feedback. The input I got from other developers is that a contextualised sample that shows how to use the APIs in a more realistic application is more appreciated.

Choosing Polymer was done as it builds on the native Web Components support and didn't require packaging like webpack or similar.

I'm all for reverting this if this is what the people using the samples feel is right.

@jan-ivar
Copy link
Member Author

jan-ivar commented Nov 1, 2018

@ErikHellman I think the samples should communicate to people with as little up-front knowledge as possible. So far we've been able to do this with plain JavaScript for the broadest possible impact, without needing dependencies on third-party libraries.

Is there a particular obstacle or problem that warrants a change from this policy? If not, I don't see why requiring people to learn and understand custom elements in order to learn how screen-sharing works, adds value, and I'd vote to revert this.

@jan-ivar
Copy link
Member Author

jan-ivar commented Nov 1, 2018

In particular, I'd hate for anyone to get the impression polymer is somehow required for screen-sharing.

@ErikHellman
Copy link
Contributor

@jan-ivar This relates to who is the target audience for these samples. Web developers with little experience from WebRTC need some kind of context. This is my experience from teaching as well as what I've heard after speaking with other web developers. In my opinion, these samples are part of the developer relations effort around WebRTC and should aim for as many developers as possible.

The choice of Polymer is simply for technical reasons around publishing this on GitHub pages. I'd love to have samples that use React, Vue.js or Angular as well, I just haven't had time to write those as well.

@jan-ivar
Copy link
Member Author

jan-ivar commented Nov 1, 2018

With all due respect, the premise that plain JavaScript doesn't reach the most web developers, seems false.

I must also agree with @fippo that changing the sample to only show screen-sharing in the context of recording, is a bad idea. Not only because it doesn't work on all browsers, but because it adds confusion about the realtime aspects of these APIs.

Recording isn't the primary use-case, sharing one's screen live in a web conference is. I'd recommend showing off the combined effect in a separate example instead.

@ErikHellman
Copy link
Contributor

I can agree that this sample could be better. The first version had some other issues as displaying the screen capture locally made for a very poor demo. I missed testing this on Edge before deploying and would have waited if I had discovered the bug there (the Polymer bug seems to be a glitch in unpkg. Looking into that separately).

I'd like to suggest two changes;

  • Change the name of this sample to better reflect what it does (e.g. Screen Recording)
  • Republish the previous version that only does screen sharing to a local video element (is it was before)

@jan-ivar
Copy link
Member Author

jan-ivar commented Nov 1, 2018

Could work, but doesn't address that polymer is unnecessary to make even this new sample run. If the new sample could live at the new url, then I suppose my interest in opposing it would be less.

@fippo
Copy link
Collaborator

fippo commented Jun 27, 2019

FWIW, unpkg breaks when trying to load locally. See https://bugs.chromium.org/p/webrtc/issues/detail?id=10768

Once upon a time these samples could be relied on.

fippo added a commit to fippo/webrtc that referenced this issue Jul 10, 2020
reverts to the state of 21e9bc3
and applies some fixes ontop of that such as the move to getDisplayMedia
and that it has to be triggered from a gesture these days.

Fixes webrtc#1148
Fixes webrtc#1152
fippo added a commit to fippo/webrtc that referenced this issue Jul 10, 2020
reverts to the state of 21e9bc3
and applies some fixes ontop of that such as the move to getDisplayMedia
and that it has to be triggered from a gesture these days.

Fixes webrtc#1148
Fixes webrtc#1152
fippo added a commit to fippo/webrtc that referenced this issue Jul 10, 2020
reverts to the state of 21e9bc3
and applies some fixes on top of that such as the move to navigator.mediaDevices
and that it has to be triggered from a gesture these days.

Fixes webrtc#1148
Fixes webrtc#1152
fippo added a commit to fippo/webrtc that referenced this issue Jul 10, 2020
reverts to the state of 21e9bc3
and applies some fixes on top of that such as the move to navigator.mediaDevices
and that it has to be triggered from a gesture these days.

Fixes webrtc#1148
Fixes webrtc#1152
Fixes webrtc#1154
fippo added a commit to fippo/webrtc that referenced this issue Jul 13, 2020
reverts to the state of 21e9bc3
and applies some fixes on top of that such as the move to navigator.mediaDevices
and that it has to be triggered from a gesture these days.

Fixes webrtc#1148
Fixes webrtc#1152
Fixes webrtc#1154
fippo added a commit to fippo/webrtc that referenced this issue Aug 7, 2020
reverts to the state of 21e9bc3
and applies some fixes on top of that such as the move to navigator.mediaDevices
and that it has to be triggered from a gesture these days.

Fixes webrtc#1148
Fixes webrtc#1152
Fixes webrtc#1154
KaptenJansson added a commit that referenced this issue Aug 21, 2020
reverts to the state of 21e9bc3
and applies some fixes on top of that such as the move to navigator.mediaDevices
and that it has to be triggered from a gesture these days.

Fixes #1148
Fixes #1152
Fixes #1154

Co-authored-by: Christoffer Jansson <cjansss@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants