Questions about the YouTube player_api code


#210

Too many cooks, and not enough work spec. I was the one helping on the other forum, and it seems, @Paul_Wilkins, that we have been undoing each other’s work and rebuilding, based on what we believed to be the coding spec.

Sounds to me like this particular implementation of the YouTube player API thing has grown in bizarre and contradictory directions. If it were mine, I would seriously start from scratch, building the smallest building blocks, and go up from there.

Just my own 0.02.


#211

I had a question about destructuring, but that can wait til this part is figured out first.
Once it’s working, if I have questions after I’ll ask them. But I think this is the only other part that needs to be adjusted, and then it should be good.

function combineSettings(oldSettings, newSettings) {

// And then stuff in here:

return Object.assign(newSettings, oldSettings);
 }

#212

This new one that’s being worked on will be set up differently from how both of the others were. Both of the others look very similar to each other, but clearly there are differences that set them apart.

I tried to figure out on my own what would go inside this new one, but it seems too confusing. I know what’s supposed to be happening though. There’s the old settings, and the new settings. What’s meant to happen here is, The new settings inside the loadPlayer are supposed to be caught by the old settings to form one setting. If I’m right about this.

New One:
https://jsfiddle.net/zt7anuL3/56/

function combineSettings(oldSettings, newSettings) {

// And then stuff in here:

return Object.assign(newSettings, oldSettings);
 }

It’s not being done this way:
https://jsfiddle.net/hzyrfkwb/553/

    function combineSettings(oldSettings, newSettings) {
        const props = Object.keys(oldSettings);
        return props.reduce(function combine(combined, prop) {
            if (oldSettings[prop].toString() === "[object Object]") {
                const oldProp = oldSettings[prop] || {};
                const newProp = newSettings[prop] || {};
                combined[prop] = combineSettings(oldProp, newProp);
            } else if (newSettings.hasOwnProperty(prop)) {
                combined[prop] = newSettings[prop];
            }
            return combined;
        }, oldSettings);
    }

It’s not being done this way:
https://jsfiddle.net/zt7anuL3/41/

  function combineSettings(oldSettings, newSettings) {
        const props = Object.keys(newSettings);
        let combinedSettings =  props.reduce(function combine(combined, prop) {
            if (typeof(oldSettings[prop]) === "object") {
                const oldProp = oldSettings[prop] || {};
                const newProp = newSettings[prop] || {};
                combined[prop] = combineSettings(oldProp, newProp);
            } else {
                combined[prop] = newSettings[prop];
            }
            return combined;
        }, oldSettings);

        return combinedSettings;
    }

#213

Which of the new settings is combineSettings going to combine? Only settings that are already specified in the default settings, or is it all of the new settings that overwrite the defaults, even if those new settings were never supposed to exist in the first place?


#214

Our last updated version:
https://jsfiddle.net/zt7anuL3/56/


function combineSettings(oldSettings, newSettings) {

// And then stuff in here:

return Object.assign(newSettings, oldSettings);
 }


    function initPlayer(wrapper) {
        const video = wrapper.querySelector(".video");
        const width = opts.width || 198;
        const height = opts.height || 198;
        const playerVars = opts.playerVars || opts;
        delete playerVars.width;
        delete playerVars.height;
        videoPlayer.init(video, {width, height, playerVars});
    }

We were fixing this one:
https://jsfiddle.net/zt7anuL3/41/

In which you said this:

I designed it to protect oldSettings from strange and unknown properties in newSettings.
The change that someone made, is to let everything in newSettings change what’s in oldSettings, no matter if it’s good,bad, or ugly.

You don’t need the combineSettings function to achieve that. You can just use Object.assign instead.

    function combineSettings(oldSettings, newSettings) {
        // We want to add or update only those properties found on our 
        //  newSettings object -- this is our custom settings. So get ONLY
        //  those keys.
        /*
        const props = Object.keys(newSettings);
        let combinedSettings = props.reduce(function combine(combined, prop) {
            // Is the current property a  nested object? If so, RECURSE!
            if (typeof(oldSettings[prop]) === "object") {
                const oldProp = oldSettings[prop] || {};
                const newProp = newSettings[prop] || {};
                combined[prop] = combineSettings(oldProp, newProp);
            } else {
                // Otherwise, simply add/update the property on  our defaults
                combined[prop] = newSettings[prop];
            }
            return combined;
        }, oldSettings);
        return combinedSettings;
        */
        return Object.assign(newSettings, oldSettings);
    }

#215

We’re combing the new settings with the old settings.

Whatever is not added to the default settings, would be the new settings.

These are the default settings: old settings

            playerVars: {
                autoplay: 1,
                controls: 1,
                showinfo: 1,
                rel: 0,
                iv_load_policy: 3,
                cc_load_policy: 0,
                fs: 0,
                disablekb: 1,
            },

These are the new settings:

loadPlayer({
    start: 200,
    end: 205,
    loop:true
});

#216

It would be this one then:

all of the new settings that overwrite the defaults, even if those new settings were never supposed to exist in the first place

These would be considered the new settings:

loadPlayer({
    start: 200,
    end: 205,
    loop:true
});

Which were never specified in the defaults:

            playerVars: {
                autoplay: 1,
                controls: 1,
                showinfo: 1,
                rel: 0,
                iv_load_policy: 3,
                cc_load_policy: 0,
                fs: 0,
                disablekb: 1,
            },

That’s how it was implemented here:
https://jsfiddle.net/zt7anuL3/41/

But you said to abbreviate, it can be achieved a better way.

And this below was part of your update to the code.

Our last updated version:
https://jsfiddle.net/zt7anuL3/56/

function combineSettings(oldSettings, newSettings) {

// And then stuff in here:

return Object.assign(newSettings, oldSettings);
 }


    function initPlayer(wrapper) {
        const video = wrapper.querySelector(".video");
        const width = opts.width || 198;
        const height = opts.height || 198;
        const playerVars = opts.playerVars || opts;
        delete playerVars.width;
        delete playerVars.height;
        videoPlayer.init(video, {width, height, playerVars});
    }

#217

When dealing with a flat set of object properties there are better ways, but when dealing with a more deeply nested set of object information, there don’t seem to be better ways.


#218

What exactly are you telling me here?

There is nothing that can be changed, or improved in this function?

This piece wouldn’t get added in then?
That was part of your new way of doing it, but now you say it can’t be done.
return Object.assign(newSettings, oldSettings);

https://jsfiddle.net/zt7anuL3/68/

function combineSettings(oldSettings, newSettings) {
    const props = Object.keys(newSettings);
    const combinedSettings = props.reduce(function combine(combined, prop) {
      if (typeof(oldSettings[prop]) === "object") {
        const oldProp = oldSettings[prop] || {};
        const newProp = newSettings[prop] || {};
        combined[prop] = combineSettings(oldProp, newProp);
      } else {
        combined[prop] = newSettings[prop];
      }
      return combined;
    }, oldSettings);

    return combinedSettings;
  }

#219

What can I say? It is after all mostly my recent work :slight_smile:


#220

This is what you did:
https://jsfiddle.net/hzyrfkwb/553/

function combineSettings(oldSettings, newSettings) {
        const props = Object.keys(oldSettings);
        return props.reduce(function combine(combined, prop) {
            if (oldSettings[prop].toString() === "[object Object]") {
                const oldProp = oldSettings[prop] || {};
                const newProp = newSettings[prop] || {};
                combined[prop] = combineSettings(oldProp, newProp);
            } else if (newSettings.hasOwnProperty(prop)) {
                combined[prop] = newSettings[prop];
            }
            return combined;
        }, oldSettings);
    }

And this is what the other function looks like:
https://jsfiddle.net/zt7anuL3/68/

function combineSettings(oldSettings, newSettings) {
    const props = Object.keys(newSettings);
    const combinedSettings = props.reduce(function combine(combined, prop) {
      if (typeof(oldSettings[prop]) === "object") {
        const oldProp = oldSettings[prop] || {};
        const newProp = newSettings[prop] || {};
        combined[prop] = combineSettings(oldProp, newProp);
      } else {
        combined[prop] = newSettings[prop];
      }
      return combined;
    }, oldSettings);

    return combinedSettings;
  }

#221

Yes, thus the “mostly”.


#222

I want to make a destructuring version of this one: #204
https://jsfiddle.net/zt7anuL3/68/

Following your steps these get deleted:

const width = opts.width || 198;
    const height = opts.height || 198;
    const playerVars = opts.playerVars || opts;
    delete playerVars.width;
    delete playerVars.height;

#223

Can you please explain what you mean with destructuring?


#224

What you explained here:

If you want to use destructuring, then the function parameters is where that’s used instead.

    function addVideo(video, settings) {
    function addVideo(video, {width, height, playerVars}) {
        // achieves the same as:
        // const width = settings.width;
        // const height = settings.height;
        // const playerVars = settings.playerVars;

        const defaultSettings = {
            // width: settings.width || 600,
            width: width || 600,
            // height: settings.height || 338,
            height: height || 338,
            ... // and so on

#225

Destructuring is useful when you are accessing a lot of properties from an object, within the same function.
It also helps if you don’t need to pass on that object to other functions either.

I don’t think that any of your code qualifies for those benefits.


#226

I tried it here but it didn’t work, how come?
https://jsfiddle.net/zt7anuL3/69/

TypeError: videoPlayer is undefined


#227

That addVideo function passes on the settings object to other functions.

So, when you destructure the object, you must also recreate that object once again afterwards.
That’s a waste, and is confusing. Your code doesn’t benefit from destructuring.

I only supplied that example code to provide a demonstration of how the technique actually works. I was not recommending that it actually gets done with your code.

Don’t do it.

Is that clear enough?


#228

What if I was only using a grid player, and that’s it, how would I get that to work?
https://jsfiddle.net/zt7anuL3/72/


#229

When you find a situation for which the criteria fits, then I’ll be happy to help you work through the process.

Here is the criteria: