fix: apply content prop changes to the running player - #41
Open
jwbrandon wants to merge 3 commits into
Open
Conversation
The setup config was built once in the constructor and never reapplied, so changing playlist or file left the old media playing. Consumers had to force a remount with a changing key. Push content changes through the player's load() method from componentDidUpdate, comparing playlist by value so a fresh but equal array does not reload. Build the setup config from the props current when setup runs, so props that change while the library loads are no longer lost. Event listeners were re-subscribed inside shouldComponentUpdate, which then returned false. That dropped any content change arriving in the same update — the common case, since inline handlers get a new identity on every parent render. Move that work to componentDidUpdate. The test asserting shouldComponentUpdate returns false for event-prop changes now expects true, matching the corrected behavior. Other config options are still setup-time only; document that and the key-remount workaround. Closes #20
Reloading a changed `file` meant rebuilding a playlist item from the top level config, which silently drops the sibling item options this package forwards without knowing their schema — image, tracks, title. Drop `file` from the live path and document it as setup-time only, alongside every other config option. Also ignore an empty or absent playlist, so content that arrives later cannot blank out a player that is already playing.
Carries over the stronger checks from the parallel PR on this issue: unit tests for deepEqual, exact assertions on load's call list rather than "was called with", a structural-equality case with nested tracks, and a table over every empty playlist shape. Also declares load() on the player API type, and documents that once<Event> handlers are subscribed at mount only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The setup config was built once in the constructor and never reapplied, so changing
playlistleft the old media playing — consumers had to force a remount with a changingkey. Playlist changes now go through the player'sload()method fromcomponentDidUpdate, compared by value so an equal-but-new array does not reload, and ignored when empty so late-arriving content cannot blank out a playing player.Two related fixes fell out of it: the setup config is now built from the props current when setup runs, so props changing during the library load are no longer lost; and event-listener re-subscription moved out of
shouldComponentUpdate, which returnedfalseafterwards and dropped any playlist change in the same update — the common case, since inline handlers get a new identity every parent render. Keeping the side effect out ofshouldComponentUpdatealso honours React's requirement that it stay pure: a render that React starts and then discards must not have already told the player to load.fileis deliberately excluded from the live path: reloading it means rebuilding a playlist item, which would drop sibling top-level item options such asimageandtracks. It and every other config option stay setup-time only, which the README now documents along with thekey-remount workaround.Supersedes #40, and carries over its stronger test coverage and its
load()type declaration.Closes #20