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

Make es5-shim a dependency #1687

Closed
6 of 10 tasks
gkatsev opened this issue Nov 24, 2014 · 12 comments
Closed
6 of 10 tasks

Make es5-shim a dependency #1687

gkatsev opened this issue Nov 24, 2014 · 12 comments

Comments

@gkatsev
Copy link
Member

gkatsev commented Nov 24, 2014

For v5, we should consider making es5-shim a dependency that users of videojs need to include for older browser support. It will make our lives easier if we can assume a higher level of js support provided by ES5.

This would also mean that we probably don't need as many other helper functions that re-introduce array iterator functions.

Remaining Tasks

  • Confirm this direction
  • Choose the CDN-hosted version to use
  • Update all guides and examples to include
    • Getting started guide
    • sandbox example files
    • build/demo files
    • Website example embed codes Moved to videojs/videojs.comj#29
    • jsbin.com/axedog This will come after 5.0 is officially release
    • What else ??? Nothing.
  • Identify areas of the codebase that could be improved with ES5 features, and create issues to update them (:mans_shoe: :football:)
@gkatsev gkatsev added this to the v5.0.0 milestone Nov 24, 2014
@mmcc
Copy link
Member

mmcc commented Nov 24, 2014

👍

@heff
Copy link
Member

heff commented Nov 24, 2014

I'm cool with this, but it doesn't free us from having additional IE8-specific code in the project (e.g. events). We've also talked about trying to have a separate file that contains all IE8-specific code, so it could be included or not. It might start to feel like a lot if people need to include the ES5 shim AND our IE8 additional code. And since the es5 shim modifies built-in objects, it's probably not something we should just include in our IE8 file.

I'd also be interested to know how this overlaps with lodash, and if we'd no longer need one or the other.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 24, 2014

What I meant specifically is that people need to add es5-shim themselves if they're using videojs in browsers that are not es5-compliant.

@heff
Copy link
Member

heff commented Nov 24, 2014

Yeah, I got that, I was just looking for ways to make it less complicated for people needing IE8 support, assuming we also introduced two different builds.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 24, 2014

Yeah, for the IE8 stuff, we probably should just have a video.js and a video.ie8.js which is regular videojs+IE8 stuff.
Anyway, the IE8 discussion I think belongs in a different thread.

As for lodash, we probably won't need many of the things lodash provides and many of the current functions we have. The main one we'd need is extend. And for that, we could also consider asking for an es6-shim to be included as well for Object.assign.

We could also provide a build that includes es5-shim in with videojs.

@mmcc
Copy link
Member

mmcc commented Nov 24, 2014

re: the Lodash thread, if we're talking about an ES5 shim, something like 101 might be pretty interesting to keep things thin without needing a custom lodash build.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 24, 2014

For lodash, I would want to use the node-style require module (require('lodash/modern/objects/assign')) or the function-per-module modules of just what we need (require('lodash.assign')).

@heff heff changed the title Consider making es5-shim a dependency Make es5-shim a dependency Apr 26, 2015
@heff
Copy link
Member

heff commented Apr 26, 2015

I updated this issue to list steps needed to complete it.

@dmlap
Copy link
Member

dmlap commented Apr 27, 2015

Decision: Bundle es5-shim, es5-sham, and html5 video/audio shim into a file that gets hosted on the video.js CDN.

Watch out for cross-browser issues when not using the shim, because it also improves support in modern browsers, though we'd like to avoid making it a requirement for everyone to include even if they're not trying to support ie8.

@ljharb
Copy link
Contributor

ljharb commented Apr 27, 2015

If you're looking for an "assign" shim that's spec-compliant, https://www.npmjs.com/package/object.assign is what you want (it will only shim the environment if you explicitly ask it to).

Please do bundle the es5-sh*m in a way that makes it easy to deliver updates.

Also, please note that every single browser, including latest Chrome and IE, lacks 100% ES5 compliance, so everything needs the es5-shim, IE 8 or no. :-)

@heff
Copy link
Member

heff commented Apr 27, 2015

Thanks for the note @ljharb. I think we still want to avoid requiring everyone to include the es5 sh*m for all browsers, so we'll have to be conservative with the features we use.

heff added a commit to heff/video.js that referenced this issue May 28, 2015
IE8 compatiblity fixes - Babel loose mode and ES5-shim

Reverted to old isPlainObject to fix IE8

Lodash.isplainobject was throwing a "Member not found error" on elements,
specifically the 'custom' track element being passed in options.

(turned out to be that we were using lodash modern, not compat)

Fixed full-window mode in IE8 by fixing fullscreen API check

Fixed the swf events by creating the object in component.createEl
fixes videojs#2184

Added es5 shim and sham to the sandbox example
related to videojs#1687
@heff
Copy link
Member

heff commented Jun 6, 2015

Closing as all tasks are complete or moved or punted.

@heff heff closed this as completed Jun 6, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants