init options bug

[mum-never-proud]

Link to related issue (if applicable)

#1737

Summary of proposed changes

this PR helps in prioritizing the config provided by the User, rather than applying values from local storage, the high priority is given to

• user-config
• local-storage
• app's default config

it also normalizes options such as quality and speed such that if the user forgets to input one of the required fields it uses app's default config for that field

this code has been tested with HTML5, Youtube and Vimeo and works without issues

Checklist

• Use develop as the base branch
• Exclude the gulp build (/dist changes) from the PR
• Test on supported browsers

[sampotts]

Why do we need this as a getter?

[mum-never-proud]

my bad we don't need a getter, but the current implementation is unreadable as well, it confuses the editor that the code below this line is not reachable

I guess we can have a method say parseJSON(jsonString) to handle this

[sampotts]

Yeah, it could just be a util really.

[sampotts]

Why not use Object.keys(options).includes(key) ?

[mum-never-proud]

hasOwnProperty is faster with fewer properties say 100, I don't think there's gonna be more than 100 keys here (as of now)

[sampotts]

Readability > a probably very minor speed increase. You'll notice the rest of the code we use that style so let's stick with that. It's the sort of thing browsers will optimise anyway.

[sampotts]

Is this not just merging the two objects? In which case we can just use the extend helper in the object utils?

[mum-never-proud]

yep, it is! We have two options with multiple properties quality and speed

the current implementation will break if the initialization is like

const player = new Plyr('#player', {
    speed: {
        selected: 2
    }
});

it will break here since the user has not provided the options

plyr/src/js/controls.js

Line 1057 in 39558cb

this.options.speed = this.options.speed.filter(o => o >= this.minimumSpeed && o <= this.maximumSpeed);

the above code just makes sure that incorrect options don't break the code, it wasn't breaking previously since we were overriding user's config

[sampotts]

What's the reason behind this change? You could potentially overwrite an existing value rather than merge.

[mum-never-proud]

I felt like extending an object should extend it unconditionally, rather than checking for the existence of keys in loop

[sampotts]

Did you test it? From looking at it, happens with your change will be that if the target already includes key, you're going to wipe it to be an empty object.

i.e.

const obj1 = { a: { b: 2, c: 3 }};
const obj2 = { a: { d: 4, e: 5 }};
const obj3 = extend(obj1, obj2); 

You would expect obj3 to be { b: 2, c: 3, d: 4, e: 5 } but with your change it'll be { d: 4, e: 5 } because it wipes out the previous value.

[mum-never-proud]

let me know your thoughts on this @sampotts

[mum-never-proud]

yep, it is! We have two options with multiple properties quality and speed

the current implementation will break if the initialization is like

const player = new Plyr('#player', {
    speed: {
        selected: 2
    }
});

it will break here since the user has not provided the options

plyr/src/js/controls.js

Line 1057 in 39558cb

this.options.speed = this.options.speed.filter(o => o >= this.minimumSpeed && o <= this.maximumSpeed);

the above code just makes sure that incorrect options don't break the code, it wasn't breaking previously since we were overriding user's config

[mum-never-proud]

my bad we don't need a getter, but the current implementation is unreadable as well, it confuses the editor that the code below this line is not reachable

I guess we can have a method say parseJSON(jsonString) to handle this

[mum-never-proud]

hasOwnProperty is faster with fewer properties say 100, I don't think there's gonna be more than 100 keys here (as of now)

[mum-never-proud]

I felt like extending an object should extend it unconditionally, rather than checking for the existence of keys in loop