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

Introduce Player::destroy #167

Merged
merged 5 commits into from
Mar 27, 2018
Merged

Introduce Player::destroy #167

merged 5 commits into from
Mar 27, 2018

Conversation

ghost
Copy link

@ghost ghost commented Oct 1, 2017

Sometimes we have to cleanup and start from scratch. For that a destroy method can come in handy.

The introduced method will remove any references to the current player and destroy a possibly existing iframe.

This PR is inspired as a workaround to " Player unusable after error at instantiation #166" which makes it impossible to use the player on the same iframe, if the player couldn't be instantiated.

This allows a player to be removed and a new one created.
Asking a destroyed player to execute functions will just fail.
@CLAassistant
Copy link

CLAassistant commented Oct 1, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ LoveIsGrief
❌ JamesHenry
You have signed the CLA already but the status is still pending? Let us recheck it.

@mirague
Copy link

mirague commented Nov 12, 2017

Would love to see this functionality. Having a way to properly reset to the initial state, with no saved/cached side-effects, would allow us to create reliable apps with this package. Currently it's way too unstable and errors happen left and right with not much for us to do about (apart from forking it and addressing these issues ourselves).

For this PR I suppose we just need to update the README.me to include destroy.

Also changed the description of unload in order to distinguish from destroy.
@ghost
Copy link
Author

ghost commented Nov 13, 2017

As suggested by @mirague , I updated README.md.
I hope the PR is approvable now.

@JamesHenry
Copy link
Contributor

JamesHenry commented Nov 27, 2017

Thanks for submitting this @LoveIsGrief. Any idea who to ping to keep this moving?

@ghost
Copy link
Author

ghost commented Nov 27, 2017

I guess @luwes and @bdougherty ?

@mirague
Copy link

mirague commented Nov 28, 2017

Unfortunately they don't seem very active with this repository, which is a shame because it's quite close to being good and usable – it just lacks some stability.

cc @fisherinnovation @berdyshev

JamesHenry added a commit to JamesHenry/player.js that referenced this pull request Nov 28, 2017
@JamesHenry
Copy link
Contributor

JamesHenry commented Nov 28, 2017

@LoveIsGrief I think there is some missing cleanup here - if you instantiate a Player using a DOM element (that isn't a Vimeo iframe), it is currently impossible to create a new one after you have destroyed it.

For that particular use case, we end up with two references in the playerMap:

  1. The element passed into new Player
  2. The iframe that gets created by createEmbed()

The current code (before your PR) confusingly first sets this.element to be (1) in the constructor, but then quietly overwrites it with (2) within the getOEmbedData then block within readyPromise.

This means that there is currently no way to get a reference to the original element and do the necessary cleanup within your new destroy() method.

Please see my quick fork in which I have duplicated your PR and added some extra logic to address the above:

master...JamesHenry:master

@ghost
Copy link
Author

ghost commented Feb 12, 2018

Hi @JamesHenry

Sorry for the late response, I didn't see the update :/
Thanks for the code. I incorporated it into the pull request.

Hopefully the maintainers will be able to review the code.
Would you have a look @luwes and @bdougherty ?

@luwes
Copy link
Contributor

luwes commented Mar 12, 2018

I will review this soon. Thanks for your patience guys!

@luwes luwes merged commit ec8d5d3 into vimeo:master Mar 27, 2018
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.

4 participants