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

Backward Compatibility Audit #2402

Closed
misteroneill opened this issue Jul 23, 2015 · 27 comments
Closed

Backward Compatibility Audit #2402

misteroneill opened this issue Jul 23, 2015 · 27 comments
Milestone

Comments

@misteroneill
Copy link
Member

I'm doing a run-through of the 4.x documentation and objects to look for properties and methods that are missing from the 5.x work or whose fate is otherwise unclear or undocumented in the wiki. A lot of this is up for discussion, but I wanted to get it in one place where we can all see it.

Globals

Global Resolution
vjs Document

videojs Properties

Property Resolution
JSON Document
TOUCH_ENABLED Restore
cache Ignore
media Ignore
options Restore #2395
players Restore #2395
util Document No longer needed.

videojs Methods

Method Resolution
bind Document Wiki says removed, but was restored.
createTimeRange Document Wiki says removed, but was restored.
parseUrl Restore
round Restore
trim Restore
util.mergeOptions Document Now top-level.

Classes

Class Resolution
CoreObject Document
EventEmitter Restore as EventTarget
MediaTechController Document

Components

Component Resolution
SeekHandle Document Moved to CSS
SliderHandle Document Moved to CSS
VolumeHandle Document Moved to CSS

Other

Object Resolution
Component#init Remove Shim and deprecate.

Player

Property/Method Resolution
cancelFullScreen Remove Had been deprecated in 4.x.
getTagSettings Document Now static.
isFullScreen Remove Had been deprecated in 4.x.
onDurationChange Document handleTechDurationChange
onEnded Document handleTechEnded
onError Document handleTechError
onFirstPlay Document handleTechFirstPlay
onFullscreenChange Document handleFullscreenChange / handleTechFullscreenChange
onLoadStart Document handleTechLoadStart
onLoadedAllData Document handleLoadedAllData
onLoadedMetaData Document handleTechLoadedMetaData
onLoadedData Document handleTechLoadedData
onPause Document handleTechPause
onPlay Document handleTechPlay
onProgress Document handleTechProgress
onSeeked Document handleTechSeeked
onSeeking Document handleTechSeeking
onTimeUpdate Document handleTechTimeUpdate
onVolumeChange Document handleTechVolumeChange
onWaitEnd Document Removed.
onWaiting Document handleTechWaiting
requestFullScreen Remove Had been deprecated in 4.x.
@dmlap
Copy link
Member

dmlap commented Jul 23, 2015

I vote to re-export TOUCH_ENABLED and EventEmitter.

@dmlap dmlap added this to the v5.0.0 milestone Jul 23, 2015
@misteroneill
Copy link
Member Author

I'm curious why EventEmitter would be exported? It seems like the sort of thing that is outside the scope of the public API of a library like VideoJS.

@gkatsev
Copy link
Member

gkatsev commented Jul 23, 2015

I don't see why not export it. It could be useful.
Also, if it's available via getComponents, that counts as exported.

@misteroneill
Copy link
Member Author

My concern was sort of two-fold. First, it increases the surface area of the API, which increases the cost of maintenance/support. Second, that it wasn't part of the core competency of VideoJS.

It's not available via getComponent.

@dmlap
Copy link
Member

dmlap commented Jul 23, 2015

I was hoping EventEmitter would get exported because we've re-implemented it in videojs-contrib-media-sources and videojs-contrib-hls. That said, I agree it's outside the core competency of video.js and may not have broad applicability for plugins.

@misteroneill
Copy link
Member Author

Well, if it's being used in plugins, then I would revise my position and say that EventEmitter should be exported. :)

@gkatsev
Copy link
Member

gkatsev commented Jul 23, 2015

Our event emitter implementation is the bare minimum and it is required for the text track work. There won't be any changes to it.

@heff
Copy link
Member

heff commented Jul 23, 2015

Can we at least rename EventEmitter to MockEventTarget or something else that has less expectations behind it? EventEmitter sounds like we're building all our components off it but we just use it to emulate an element for tracks.

@heff
Copy link
Member

heff commented Jul 24, 2015

@misteroneill this is awesome, thanks for putting it together.

  • vjs - This wasn't exported in the minified version and shouldn't be in any docs/guides. I guess we could still make a note for anyone relying on video.dev.js.
  • JSON - Probably worth a deprecation warning since it was exported before
  • cache - No need to document. Only used internally.
  • media - No need to document. There was nothing in it.
  • parseUrl, round,trim - Re-add? With these I was hoping we'd have more time to revisit our strategy for them. Do we need them, can we pull in a module for them instead, etc.
  • *Handle - Handles live in CSS land now
  • MediaLoader - This should be there
  • Component#init - The function you mentioned can be removed. Whether or not we want to shim init is another question.

@misteroneill
Copy link
Member Author

No problem! I've updated the tables with your feedback @heff. You're right that MediaLoader is there. Not sure where that came from.

What do you mean by shimming init? Maybe keeping it around with a deprecation warning?

@dmlap
Copy link
Member

dmlap commented Jul 24, 2015

I'm fine with renaming the EventEmitter but can we avoid using the adjective "Mock"? That's strongly associated with testing in my mind and the EventEmitter is intended for more than just unit testing.

@gkatsev
Copy link
Member

gkatsev commented Jul 24, 2015

I came here to say I could be convinced to rename EventEmitter to EventTarget but @dmlap basically said what I was thinking.

@gkatsev
Copy link
Member

gkatsev commented Jul 24, 2015

@misteroneill btw, vjs 4.0 had a bunch of helper functions that were available in the dev version (but not the minified version due to GCC's minification). For example videojs.obj.isArray. Would be great if you can find what we had previously.

@gkatsev
Copy link
Member

gkatsev commented Jul 24, 2015

@misteroneill in 5.0 we're no longer using Component's init. That was our old way of specifying constructors. So, if we want to keep it, we'll need to shim it so that the old init works like our new constructor method. See https://github.com/videojs/video.js/wiki/5.0-Change-Details#switched-to-es6-classes-updated-how-you-subclass-components

@heff
Copy link
Member

heff commented Jul 24, 2015

(man, I'm totally gonna change my username to misterheff. It makes every response sound so respectful!)

👍 EventTarget

There's levels of backwards compatibility we can support for component initialization. init alone wouldn't be hard. I'll try to summarize the rest later.

@misteroneill
Copy link
Member Author

I demand formality in all things! (Actually, it's just that I have a common name/first initial, so a bunch of usernames and domains have been long-registered)

@gkatsev Interesting re: videojs.obj.* and similar. I've added a few of these from the unmangled version of VJS 4.x in case there's interest in restoring them. I'll keep digging.

@misteroneill
Copy link
Member Author

Ok, so, in going through 4.x's lib.js it looks like the unmangled version had a lot of functions that are not present in 5.0. Does it make sense to document each of these... or not?

@gkatsev
Copy link
Member

gkatsev commented Jul 24, 2015

Most of the stuff wasn't exported, so, documentation isn't necessary. However, a lot of those are super useful and we definitely should export them.
Stuff like videojs.obj.isArray however isn't necessary because for 5.0 we assume es5 or es5-shim, so, it should be available globally.

@misteroneill
Copy link
Member Author

Alright, in that case, let's make a separate issue for things that were previously not exported and would be nice to export in 5.0. I don't want to lose focus of the purpose of this ticket - finding methods of videojs and Player that were lost in the transition to 5.0.

@misteroneill
Copy link
Member Author

For init support, I'm thinking the simplest path forward is something like:

if (typeof subClassMethods.init === 'function') {
  log.warn('The "init" method is deprecated in favor of "constructor".');
  subClassMethods.constructor = subClassMethods.init;
  delete subClassMethods.init;
}

Opinions?

@imbcmdth
Copy link
Member

Time to put my 🚲 in the shed. EventTarget sounds like something that receives events.. maybe EventSource? 😃

@gkatsev
Copy link
Member

gkatsev commented Jul 24, 2015

EventTarget is the name the DOM uses for it's EventEmitter implementation that DOM Elements extend from.

@imbcmdth
Copy link
Member

@gkatsev I concede.

@gkatsev
Copy link
Member

gkatsev commented Jul 24, 2015

I still would prefer to keep EventEmitter.

@misteroneill
Copy link
Member Author

Updated wiki document to reflect these changes/removals.

@misteroneill
Copy link
Member Author

Upon further investigation, trim does not exist in the codebase anymore, so I'm changing it to Remove from Restore.

@misteroneill
Copy link
Member Author

Closing this in favor of PR #2406. Changes are moved to the wiki, which is the canonical source for changes/removals instead of this ticket.

@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.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants