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

*Announcement*: v3 beta testing #766

Closed
sampotts opened this issue Jan 23, 2018 · 38 comments
Closed

*Announcement*: v3 beta testing #766

sampotts opened this issue Jan 23, 2018 · 38 comments
Assignees

Comments

@sampotts
Copy link
Owner

sampotts commented Jan 23, 2018

Hey,

I'm happy to say that the illusive v3 is now available for beta testing. I'm sorry it's taken so long but it turned into a full blown re-write in the end. The changelog is massive as you can imagine and there's quite a few breaking changes. The docs are also updated.

It also includes an ads plugin (in partnership with https://vi.ai) which we are in the final stages of finalising. It's currently undocumented but once we're happy with it then it'll be documented and publicised accordingly.

If you find any bugs then please prefix them with [v3] so I know it's the new version. Once all testing is complete and bugs fixed, then it'll go to master.

There's a preview up here:
https://plyr.io/beta

Also, if you have any questions, feel free to duck into the slack workspace

Cheers
Sam

@sampotts sampotts changed the title Announcement: v3 beta testing *Announcement*: v3 beta testing Jan 23, 2018
@panoti
Copy link

panoti commented Jan 23, 2018

Do you intend to support quality level selector for hls source? and I can't add custom control by function passing in controls option. Is it a bug?

@sampotts
Copy link
Owner Author

I'll have to investigate the HLS option. It won't be in v3 but can follow after.

With the controls issue, I'll have to take a look. Will get back to you.

@adantzler
Copy link

Congratulations man! This is looking really great! Just a note to say we are thankful for people like you that willing to take the time to build this. We are looking at implementing this into a new e-learning platform we are building called LearnSocially.

@dacostafilipe
Copy link

Great work! I integrated this in one of our sites and seems to work fine.

@friday
Copy link
Collaborator

friday commented Jan 29, 2018

At this time you also have to set iconUrl to get the updated svg icon sprite containing for the settings. Ex:

new Plyr(video, {iconUrl: 'https://cdn.plyr.io/3.0.0-beta.8/plyr.svg'});

@friday friday mentioned this issue Jan 29, 2018
6 tasks
@sampotts
Copy link
Owner Author

Oh good catch @friday . I'll address that.

@sampotts
Copy link
Owner Author

sampotts commented Jan 30, 2018

The icon url is fixed in the latest version. I wasn't updating the defaults.js as part of my deployment scripts. Resolved now 👍

Still need to look at the controls issue. Will do ASAP.

@omzi
Copy link

omzi commented Jan 30, 2018

Nice update!

@intelligence
Copy link

Nice work Sam! Can't wait to use this on production sites!

@aolko
Copy link

aolko commented Feb 1, 2018

slack bit.ly link is dead :(

@dacostafilipe
Copy link

This new version does not seem to work on Internet Explorer without the polyfills.

Maybe this should be added to the documentation?

@sampotts
Copy link
Owner Author

sampotts commented Feb 1, 2018

Sorry, will check the bit.ly link.

Yep, the new version uses ES6 which IE obviously doesn’t support so the polyfills are required if you need to support IE. Will add a section on polyfilling. It’s going to be the case with most libs these days.

@dacostafilipe
Copy link

@sampotts Yeah, makes sense, but I use your lib on computers that don't have access to the internet. Would it make sense to add a "build with polyfills" option to your build process?

@aolko
Copy link

aolko commented Feb 1, 2018

wait, how about babel? Should work.

@friday
Copy link
Collaborator

friday commented Feb 1, 2018

@dacostafilipe, @sampotts, @aolko

I've been thinking about this as well. For example, fetch was added as a plyr dependency after I initially started using it. I'm not using fetch due to the lack of some features XHR handles. It also isn't part of any of the es6/7/8-sets on polyfill.io. So when I updated it broke IE support.

It's beta, so these things should be expected. But it would be nice to ensure that future stable releases don't require additional undocumented polyfills. If I understand it correctly eslint-plugin-compat can be used to make the build fail if the additional polyfills aren't explicitly listed in a whitelist. In that case this could be used to make the dev aware of any additional polyfill requirements introduced since last release (tried it now, but the plugin is based on a limited blacklist and can't be trusted to report all or even most browser incompatibilities)

This could in turn be used to document additional requirements in the changelog, release notes and main page. But also possibly to maintain an always updated set of polyfills needed at polyfill.io. Blissfuljs for example does this. I think this makes it much easier for devs.

Alternatively, building with polyfills included as suggested by @dacostafilipe makes sense too. This should be less complicated with babel, but adds more of an overhead. I'm not sure how well babel and rollup actually handles pruning unused polyfills.

@sampotts
Copy link
Owner Author

sampotts commented Feb 1, 2018

I've been working a lot with ES6 recently and haven't really found an ideal solution to this. polyfill.io means relying on a third party which is never ideal and babel-polyfill / core-js massively bloat the bundle for 95% of users who don't need the polyfills. I also found babel-polyfill included a fair bit I didn't need even with it set up correctly to apparently include all that's required rather than everything. My plan, as lazy as it sounds, was to let the end users decide since inevitably you're going to have to polyfill for other libs as ES6 and newer become more prevalent. I could probably do a polyfilled bundle somehow with gulp/rollup but it was like 50k gzipped when I tried which is insane.

I can probably roll back fetch to use XHR as I'm not sure I need fetch as such anyway. Polyfill.io does include a polyfill but it's 7k and as I say, I'll work on replacing it with XHR.

@sampotts
Copy link
Owner Author

sampotts commented Feb 3, 2018

Slack link updated ... it's https://bit.ly/plyr-slack 👍

@Maqsyo
Copy link

Maqsyo commented Feb 5, 2018

i have Problems with fetch too
-> Safari 10.1 (mobile) doesn't know this function
-> core-js won't like to implement a polyfill

Also there's a Problem with "find" -> IE11 doesn't support this function. But for this problem i can use core-js

v3.0.11-beta
used with babel and ES6

@sampotts
Copy link
Owner Author

sampotts commented Feb 5, 2018

I'll document the polyfills but it's pretty common practice now with ES6. As I mentioned above, you'll notice the demo uses polyfill.io...

@sampotts
Copy link
Owner Author

sampotts commented Feb 5, 2018

OK, just pushed 3.0.0-beta.12 with:

  • Fetch removed in favour of XHR. One less polyfill. Didn't really need fetch anyway.
  • Added more notes about polyfilling
  • Added backwards compatibility for <div> embed placeholders

Next up is to fix captions on IE11 and the custom controls.

For anyone that wants the easy polyfill option...

<script src="https://cdn.polyfill.io/v2/polyfill.min.js?features=es6,Array.prototype.includes,CustomEvent"></script>

I'll make a more comprehensive list of the polyfill features required at some point before release but that string works well for now.

@dacostafilipe I will look at making a polyfilled built JS option. This should be possible.

@aolko Babel can handle polyfills and that's probably what I'll end up using for the polyfilled build. The problem seems to be most of their polyfills are huge. It at least doubled the size of Plyr when I tried it. Bit of a shame when ~90% of users won't need most of the polyfills.

@sampotts
Copy link
Owner Author

Hey,

I've just pushed v3.0.0-beta.13 which contains:

  • A fix for custom controls
  • Completely rewritten fullscreen class - iOS native fullscreen is WIP since playsinline makes it a little more tricky
  • A bug fix from @friday

Will look at IE11 captions next.

Cheers 👍

@gehaktmolen
Copy link
Contributor

following polyfills could fix the fetch and promise problems:
npm install whatwg-fetch --save-dev
npm install es6-promise --save-dev

@friday
Copy link
Collaborator

friday commented Feb 12, 2018

@gehaktmolen: Fetch isn't used anymore.

@dacostafilipe
Copy link

After @sampotts post regarding polyfills, I did some tests of my own.

I saw a 75% overhead when using babel-polyfill, obviously, this is not usable in production. (think about the idea of all libs/frameworks including polyfills ... scary)

For now, we will keep v2 for our "offline" sites and use v3 + polyfill.io on our online sites.

That said, I would add an build option for v3+polyfills for those special cases. I could create a PR if needed.

@sampotts
Copy link
Owner Author

Yeah I will work out a build that includes polyfills. I'll probably try and use core-js and include just the polyfills that are needed or use babel-polyfill and babel-preset-env to try and limit the polyfills. I'll work something out...

@sampotts
Copy link
Owner Author

sampotts commented Feb 17, 2018

Just pushed v3.0.0-beta.14 which contains:

  • Fix for pause button not showing while playing
  • Added polyfilled build (need to document) as plyr.polyfilled.js - this is 20kb (gzipped!) larger than the base build so I'd warn that it's only really suitable for standalone players as really you'd want to manage polyfills globally
  • Added unminified builds to /dist - minified is now plyr.min.js (need to document). CDN builds remain minified and use the same filename (plyr.js)

Will look at IE11 captions today or tomorrow.

@sampotts
Copy link
Owner Author

Oh, thanks to @friday for the work on the unminified builds 👍

@friday
Copy link
Collaborator

friday commented Feb 17, 2018

Glad I could help :)

Forgot to mention, if someone has gotten a [object%20Object] network request error (trying to get the icon svg), then this could be due to uglifyjs. It's not really a plyr bug since the error is introduced later. It has only happened to me using uglifyjs with mangle enabled (the default) and using the minified plyr build as a source. Either using the non-minified build which is now available in v3.0.0-beta.14 as dist/plyr.js, disabling mangle or switching to uglify-es (although it's significantly slower) seems to work.

@mariusgnicula
Copy link

Awesome job @sampotts !! Really appreciate this player, man! I saw you added a quality switcher for Youtube, any idea when that's coming for HTML5? Thanks again, bro!

@friday
Copy link
Collaborator

friday commented Feb 19, 2018

Small issue: The progress bar is growing and shrinking as the time changes. This is seemingly because the new font isn't monospace and hence digits 1 and 7 are not as wide as 0 and 8, causing noticeable jumps.

Switching back to the old font fixes it, but obviously doesn't look quite the same:

.plyr--video .plyr__time {
  font: 600 13px 'avenir';
}

You can also force the new font to be "monospace" for modern browsers it seems (this was news to me). If I understand it correctly this can also use different glyphs within the same font (and does):

.plyr--video .plyr__time {
  font-variant-numeric: tabular-nums;
}

https://css-tricks.com/almanac/properties/f/font-variant-numeric/

@sampotts
Copy link
Owner Author

sampotts commented Feb 20, 2018

This last solution seems like a good one. Good find! 👍 I'll give it a go. The new font is really only for use in the demo as that's all it's licensed for anyway.

@sampotts
Copy link
Owner Author

sampotts commented Mar 13, 2018

@friday I included your typeface change. I actually set it as a global declaration on the whole player as we present timestamps in a few spots and I actually preferred now the numerics looked with it.

3.0.0-beta.18 contained several fixes for issues with .destroy() leaving event bindings on the document causing issues (as you can imagine). Also fixes for trying to set menus when there's no menus in the controls and some improvements to the ad plugin from @friday and @gehaktmolen 👍

3.0.0-beta.19 contains the above fix for the timestamps font spacing, thanks to @friday.

@friday
Copy link
Collaborator

friday commented Mar 13, 2018

Nice! You've been working hard lately :) I think you have fixed all known v3 issues now? and obviously some that were present in v2 before.

@sampotts
Copy link
Owner Author

I think so. Closing all the tickets related to bugs fixed in v3 might take me a while. I think I'll push current master to a v2 branch and then merge beta to master tomorrow night and prepare for the backlash 😂

@Maqsyo
Copy link

Maqsyo commented Mar 13, 2018

3.0.0-beta.19 still Problems with destroy() on HTMLAudioElement

Uncaught TypeError: Cannot read property 'display' of null
    at e.timeUpdate (plyr.min.js:1)
    at HTMLAudioElement.<anonymous> (plyr.min.js:1)

@sampotts
Copy link
Owner Author

@Maqsyo Try 3.0.0-beta.20 - I found in some cases I had to delay the garbage collection of old elements.

@Maqsyo
Copy link

Maqsyo commented Mar 14, 2018

Thanks! this worked for me :)

@sampotts
Copy link
Owner Author

Thanks for all your feedback and help. I'm gonna close this for now but if you find anything, please don't hesitate to grab me in Slack or raise an issue if need be! 👍

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

No branches or pull requests