Upgrade dependencies and other improvements #853
Conversation
There was a problem hiding this comment.
There are a lot of mixed-purpose changes in this one PR, making it a difficult branch to code review. The work you've done on upgrading React Router is great though, something I've struggled to find the time for 👌
A few minor changes required, but overall looks pretty good. The key to preparing this for release will be throughly testing every component that was touched by the upgrades. Unit tests are very lacking so do not provide enough confidence without manual user testing. Once you've confirmed all is working well then we can schedule the release.
| socket.on('state:online', (data) => handleMessage(socket, store, 'state:online', data)); | ||
| socket.on('state:offline', (data) => handleMessage(socket, store, 'state:offline', data)); | ||
| socket.on('event:tracklistChanged', (data) => handleMessage(socket, store, 'event:tracklistChanged', data)); | ||
| socket.on('event:playbackStateChanged', (data) => handleMessage(socket, store, 'event:playbackStateChanged', data)); | ||
| socket.on('event:seeked', (data) => handleMessage(socket, store, 'event:seeked', data)); | ||
| socket.on('event:trackPlaybackEnded', (data) => handleMessage(socket, store, 'event:trackPlaybackEnded', data)); | ||
| socket.on('event:trackPlaybackStarted', (data) => handleMessage(socket, store, 'event:trackPlaybackStarted', data)); | ||
| socket.on('event:volumeChanged', (data) => handleMessage(socket, store, 'event:volumeChanged', data)); | ||
| socket.on('event:muteChanged', (data) => handleMessage(socket, store, 'event:muteChanged', data)); | ||
| socket.on('event:optionsChanged', (data) => handleMessage(socket, store, 'event:optionsChanged', data)); | ||
| socket.on('event:streamTitleChanged', (data) => handleMessage(socket, store, 'event:streamTitleChanged', data)); |
There was a problem hiding this comment.
What is the background to this change? Did you observe issues with listening to all emitted events?
| @@ -1,5 +1,6 @@ | |||
| /node_modules/ | |||
| /Mopidy_Iris.egg-info/ | |||
| /mopidy_iris/statc/* | |||
There was a problem hiding this comment.
Typo, should be /mopidy_iris/static/*
| @@ -10,86 +10,87 @@ | |||
| }, | |||
| "main": "app.js", | |||
| "dependencies": { | |||
There was a problem hiding this comment.
What has been the process for working through these upgrades? Each dependency upgrade needs testing in isolation (especially high-impact dependencies like react, react-dnd, react-redux, etc). Upgrading in bulk like this is not ideal. Instead I would recommend a separate branch and pull request for each group of upgrades (ie react-based dependencies in one branch).
How much testing have you performed against these upgrades?
| <Route path="modal/*" element={<Modals />} /> | ||
| <Route path="*" element={<Content />} /> |
There was a problem hiding this comment.
This is pretty neat, I like the approach here 👍
| @@ -78,7 +79,7 @@ const GridItem = ({ | |||
| type: item?.type?.toUpperCase() || 'UNKNOWN', | |||
| item: { item, context: item }, | |||
| }); | |||
| const tile = ['playlist_group', 'mood', 'directory', 'category'].indexOf(item?.type) > -1 | |||
There was a problem hiding this comment.
What is the reason for removing directory from the tile-like rendering style?
| subdirectories: subdirectoriesWithImages, | ||
| }, | ||
| }); | ||
| const playlistsLoaded = new Promise((resolve) => { |
There was a problem hiding this comment.
This may cause really heavy, un-cancellable requests for slow backends or when the directory contains a lot of playlists. Depending on the backend provider, but this could result in the fetching of the entire playlist including its tracks.
Have you tested it against the likes of Mopidy-Ytmusic or Mopidy-Tidal?
| if (data.tracks && data.tracks_total !== undefined) { | ||
| playlist.tracks_total = data.tracks_total; |
There was a problem hiding this comment.
From memory, this code was to pluck the total sub-property from the tracks object, which if provided, should trump the value of tracks.length. Most providers paginate their APIs, so the tracks.length would typically be that page, rather than the total count.
I recommend undoing this change unless there is a specific bug that relates to this.
| case 'bandcamp': | ||
| case 'Bandcamp': |
There was a problem hiding this comment.
Given this returns the same as default, is this not redundant?
| <Route path="" element={<Services />} /> | ||
| <Route path=":services/" element={<Services />} /> | ||
| <Route path=":services/:service/" element={<Services />} /> | ||
| <Route path=":services/:service/:id" element={<Services />} /> |
There was a problem hiding this comment.
If all sub-paths resolve to <Services /> couldn't we just use a wildcard path?
| @@ -12,16 +11,13 @@ import * as uiActions from '../services/ui/actions'; | |||
| import * as mopidyActions from '../services/mopidy/actions'; | |||
| import * as spotifyActions from '../services/spotify/actions'; | |||
| import { titleCase } from '../util/helpers'; | |||
| import { withRouter } from '../util'; | |||
| import { i18n } from '../locale'; | |||
|
|
|||
| class Search extends React.Component { | |||
There was a problem hiding this comment.
This whole component needs a good bit of spring cleaning. Overhauling the UX and rewriting the logic from scratch is high on my agenda.
|
@rkashapov any progress on making those review changes? |
|
@rkashapov I'm going to close this PR as I haven't had any feedback from you on this. Some of the changes have been implemented in a separate branch, so thanks for your initial contribution! |
Thank you for the great UI for mopidy!
I've upgraded the dependencies and implemented some improvements:
Sorry for the big diff