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

init options bug #1761

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mum-never-proud
Copy link

@mum-never-proud mum-never-proud commented Apr 4, 2020

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

Comment on lines +406 to +413
get dataPlayerConfig() {
try {
return JSON.parse(this.media.getAttribute('data-plyr-config'));
} catch (e) {
return {};
}
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this as a getter?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it could just be a util really.


Object.keys(defaults)
.forEach(key => {
if (Object.prototype.hasOwnProperty.call(options, key)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -0,0 +1,18 @@
export default function(options, defaults) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Comment on lines -31 to -34
if (!Object.keys(target).includes(key)) {
Object.assign(target, { [key]: {} });
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@mum-never-proud mum-never-proud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me know your thoughts on this @sampotts

@@ -0,0 +1,18 @@
export default function(options, defaults) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Comment on lines +406 to +413
get dataPlayerConfig() {
try {
return JSON.parse(this.media.getAttribute('data-plyr-config'));
} catch (e) {
return {};
}
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Object.keys(defaults)
.forEach(key => {
if (Object.prototype.hasOwnProperty.call(options, key)) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines -31 to -34
if (!Object.keys(target).includes(key)) {
Object.assign(target, { [key]: {} });
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants