implement lifecycle hooks and trigger pre/post setup hooks #3639

brandonocasey · 2016-09-23T20:33:33Z

Description

There is no current way to hook into the setup that video.js does, and this functionality would be useful for many plugins to use. (they can even initialize themselves with it)

Specific Changes proposed

Add several "hook" functions to the videojs object
Add pre/post setup lifecycle event triggers

Requirements Checklist

Feature implemented / Bug fixed
Reviewed by Two Core Contributors

gkatsev

I don't think that the hook function needs ready.
Also, would be good to add a guide documenting this feature.

gkatsev · 2016-09-23T20:51:06Z

src/js/video.js

+  }
+
+  videojs.hooks('presetup').forEach(function(hookFunction) {
+    options = videojs.mergeOptions(options, hookFunction(tag, options, ready));


Why pass ready to the hook?

we probably don't need to

gkatsev · 2016-09-23T20:52:04Z

src/js/video.js

+ * @param {String} type the lifecyle event to get hooks from
+ * @return {Array} an array of hooks, or an empty array if there are none
+ */
+videojs.hooks = function(type) {


Is hooks a too-generic term? would we want any more hooks on the main videojs object beyond presetup and postsetup?

maybe pre/post shutdown? (there is no notion of this yet though

misteroneill · 2016-09-27T15:49:47Z

src/js/video.js

+    return tag.player || Player.players[tag.playerId];
+  }
+
+  videojs.hooks('presetup').forEach(function(hookFunction) {


The ModalDialog component has similar lifecycle events, but the naming scheme is:

beforefoo: Happens before the thing.

foo: Happens after the thing.

Maybe we ought to follow that universally and use beforesetup and setup?

we could, and previously I had it implemented like that, it just seemed like a lot of duplicate code

Wouldn't changing it just just involve changing postsetup on line 111 into setup?

And presetup to beforesetup? I don't see where the duplicate code concern comes from...

These are the possible implementations with that naming scheme & the pro's/con's. do you see any other ways to implement it that sound better? (maybe I am just not seeing a good implementation?)

Also I wanted to note that using setup seem a bit weird to me since it will actually be called postSetup and not during setup

Add lifecycle setters, getters, removers, and lists in a loop to the prototype

can make things confusing

will not duplicate code

will pollute the video.js object with any lifecycle event functions that we need to add

may be more intuitive for a new user to call beforeSetup and setup to access things

use the hooks terminology internally and add lifecycle setters, getters, removers, and lists to the prototype in a loop

Makes me think we should just use hooks directly

will only pollute the video.js object with wrapper functions

will not duplicate code

may be more intuitive for the user

implement lifecycle functions without any looping or hooks terminology

Every time we need to expose a new event we have to duplicate functionality (adding a setter, getter, remover, and a list to video.js)

will pollute video.js with wrapper functions

lots of duplicate code

may be intuitive for the user

I think this is resolved

Yep, thanks!

misteroneill · 2016-09-29T18:51:59Z

docs/guides/hooks.md

@@ -0,0 +1,85 @@
+# Hooks
+Hooks exist so that users can latch on to certain Video.js lifecycle events


I think we ought to explain that this is a separate concept from per-player events. In fact, we should probably not use the term "events" at all, lest we cause confusion with events-events.

maybe something like:
Hooks exist so that users can hook into the video.js player lifecycle

Conceptually similar to events, hooks provide users a way to intercept universal behaviors of video.js itself rather than individual players.

Are they similar to events though? You don't generally have the ability to do an action like what we're doing with these hooks.

Fair enough, but they are conceptually similar in the sense that you're providing a function to be called when a certain condition is met. But, of course, I'm not married to any particular phrasing. 😄

misteroneill · 2016-09-29T18:53:37Z

docs/guides/hooks.md

+        // automatically
+        return options;
+    };
+```


I think this example (and the one below) is unclear without the videojs.hook() call.

misteroneill · 2016-09-29T18:54:56Z

docs/guides/hooks.md

+
+
+## Current Hooks
+Currently the following lifecycle events are accept and use hooks


I think the heading can go away. And the phrasing/grammar here is weird. Maybe:

Currently, the following hooks are available:

misteroneill · 2016-09-29T18:55:31Z

docs/guides/hooks.md

+Currently the following lifecycle events are accept and use hooks
+
+### beforesetup
+Called just before the player is created, so that the user can modify the options passed to the player or modify the video element that will be used for the player


Missing a period here.

I think we should mention here (and not just in the example) what happens if multiple hooks are.
Also, state that the method takes in the video element and an options object and is expected to return a new options object.

gkatsev · 2016-09-29T20:34:33Z

docs/guides/hooks.md

@@ -0,0 +1,85 @@
+# Hooks
+Hooks exist so that users can latch on to certain Video.js lifecycle events


maybe something like:
Hooks exist so that users can hook into the video.js player lifecycle

gkatsev · 2016-09-29T20:36:49Z

docs/guides/hooks.md

+Currently the following lifecycle events are accept and use hooks
+
+### beforesetup
+Called just before the player is created, so that the user can modify the options passed to the player or modify the video element that will be used for the player


I think we should mention here (and not just in the example) what happens if multiple hooks are.
Also, state that the method takes in the video element and an options object and is expected to return a new options object.

gkatsev · 2016-09-29T20:38:40Z

src/js/video.js

+  }
+
+  videojs.hooks('beforesetup').forEach(function(hookFunction) {
+    options = videojs.mergeOptions(options, hookFunction(tag, videojs.mergeOptions({}, options)));


what happens if the hookFunction returns null or a string?
We probably should write some tests for this.

It may be easier to write tests if these are moved into a separate file and then copied over onto the videojs object but either way should work.

gkatsev · 2016-09-29T20:42:27Z

src/js/video.js

+ */
+videojs.hook = function(type, fn) {
+  videojs.hooks_[type] = videojs.hooks_[type] || [];
+  videojs.hooks_[type].push(fn);


instead of using push, we should do videojs.hooks_[type] = videojs.hooks_[type].concat(fn);.
See videojs/mux.js#121

gkatsev · 2016-09-29T20:43:28Z

src/js/video.js

+ * @return {Function|undefined} the function that was removed or undef
+ */
+videojs.removeHook = function(type, fn) {
+  const hooks = videojs.hooks_[type] || [];


we should also slice here as well. See videojs/mux.js#121 again.

brandonocasey · 2016-09-29T21:21:52Z

I still have to write some unit tests for this functionality

gkatsev · 2016-10-03T18:22:02Z

src/js/video.js

+ * @param {String} type the lifecyle event to get hooks from
+ * @return {Array} an array of hooks, or an empty array if there are none
+ */
+videojs.hooks = function(type) {


Can we add a check to hooks that if it's called with two arguments either logs an error or calls hook for the user? Just ran into this problem and took a second to figure it out.

gkatsev

With proposed changes, LGTM.

gkatsev · 2016-10-03T18:23:02Z

src/js/video.js

+ * @param {Function|Array} fn the function to attach
+ */
+videojs.hook = function(type, fn) {
+  const hooks = videojs.hooks[type];


this should be videojs.hooks_[type] || [] or videojs.hooks(type);

gkatsev · 2016-10-03T18:23:32Z

src/js/video.js

+videojs.hook = function(type, fn) {
+  const hooks = videojs.hooks[type];
+
+  hooks[type].concat(fn);


should be videojs.hooks_[type] = hooks.concat(fn);

add a guide for hooks

gkatsev · 2016-10-06T15:21:53Z

src/js/video.js

+  }
+
+  videojs.hooks('beforesetup').forEach(function(hookFunction) {
+    const opts = hookFunction(tag, videojs.mergeOptions({}, options));


basically, each handler that doesn't return an object is ignored?

well they are still run, we just assume they don't want to change options

Yup, makes sense. Meant the response is ignored.

gkatsev · 2016-10-06T15:24:03Z

src/js/video.js

+  videojs.hooks_[type] = videojs.hooks_[type].slice();
+  videojs.hooks_[type].splice(index, 1);
+
+  return index > -1;


maybe also do this check above so we don't run the extra slice and splice unless necessary?

gkatsev · 2016-10-06T15:27:06Z

test/unit/video.test.js

+  assert.equal(videojs.hooks_.bar.length, 0, 'should have 0 bar hook');
+});
+
+QUnit.test('should be get all hooks for a type', function(assert) {


should be able to get all hooks for a type

done, good catch

gkatsev · 2016-10-06T15:29:21Z

test/unit/video.test.js

+  let setupCalled = false;
+  let beforeSetupCalled = false;
+  const beforeSetup = function(video, options) {
+    beforeSetupCalled = true;


what about asserting that setupCalled is still false in here?

done, also added a check for beforeSetupCalled in the setup function

gkatsev · 2016-10-06T15:31:05Z

test/unit/video.test.js

+  assert.equal(setupCalled, true, 'setup was called');
+});
+
+QUnit.test('beforesetup returns dont break videojs options', function(assert) {


Do we want a positive test case for this as well?
Something like: beforesetup hooks modify options properly and merge each item in the list

gkatsev · 2016-10-07T15:40:10Z

test/unit/video.test.js

+
+  const vid = document.getElementById('test_vid_id');
+
+  videojs.hook('beforesetup', function(options) {


can we add another hook that say adds fluid: true option?

gkatsev requested changes Sep 23, 2016

View reviewed changes

misteroneill reviewed Sep 27, 2016

View reviewed changes

gkatsev added this to the 5.13 milestone Sep 27, 2016

misteroneill requested changes Sep 29, 2016

View reviewed changes

gkatsev requested changes Sep 29, 2016

View reviewed changes

gkatsev reviewed Oct 3, 2016

View reviewed changes

gkatsev requested changes Oct 3, 2016

View reviewed changes

gkatsev added minor This PR can be added to a minor release. It should not be added to a patch release. needs: updates labels Oct 3, 2016

brandonocasey added 8 commits October 4, 2016 17:22

implement lifecycle hooks and trigger pre/post setup hooks

440a7fe

dont pass ready to presetup

a6aae25

make options default to object

f6c3194

add a guide for hooks

renamed presetup to beforesetup, and postsetup to setup

1531b4e

code review changes

a1d00eb

small logic change

74b120f

unit tests, and small code changes for them

52697fd

handle bad beforesetup retrun values

2ea6a47

misteroneill approved these changes Oct 6, 2016

View reviewed changes

misteroneill added confirmed and removed needs: updates labels Oct 6, 2016

gkatsev requested changes Oct 6, 2016

View reviewed changes

brandonocasey added 2 commits October 6, 2016 11:50

small updates from code review

849b10a

added a positive test case for beforesetup options

cddd175

gkatsev reviewed Oct 7, 2016

View reviewed changes

gkatsev approved these changes Nov 4, 2016

View reviewed changes

gkatsev merged commit 77357b1 into videojs:master Nov 4, 2016

brandonocasey deleted the feature/setup-hook branch December 6, 2016 16:25

		@@ -0,0 +1,85 @@
		# Hooks
		Hooks exist so that users can latch on to certain Video.js lifecycle events



		## Current Hooks
		Currently the following lifecycle events are accept and use hooks


		const vid = document.getElementById('test_vid_id');

		videojs.hook('beforesetup', function(options) {

implement lifecycle hooks and trigger pre/post setup hooks #3639

implement lifecycle hooks and trigger pre/post setup hooks #3639

Conversation

brandonocasey commented Sep 23, 2016

Description

Specific Changes proposed

Requirements Checklist

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add lifecycle setters, getters, removers, and lists in a loop to the prototype

use the hooks terminology internally and add lifecycle setters, getters, removers, and lists to the prototype in a loop

implement lifecycle functions without any looping or hooks terminology

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented Sep 29, 2016

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

gkatsev Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev Oct 3, 2016 •

edited

Loading

brandonocasey Oct 6, 2016 •

edited

Loading