Do not overuse state

[ghost]

In PlaylistContext.js , you are overusing state

import React, { useState,createContext } from 'react';
import myData from './data.json';
const {tracks} = myData;

export const PlaylistContext = createContext();

export class PlaylistProvider extends React.Component {
    // avoid using constructor
    // rather use class properties/methods (handled by Babel plugin "transform-class-properties")
    // state = {}
    // getListData(currentFolder, favoriteTracks) { ... }

    constructor(props){
        super(props);
        this.getListData = (currentFolder,favoriteTracks) => {
            if(favoriteTracks){
                return this.mapRecursive(tracks.children).filter((track)=>track.favorite);
            }else{
                return [...currentFolder.children];
            }
        }
        this.mapRecursive = (trackList) => {
            // This shoud be a class method
            let output = [];
            trackList.map((track)=>{
                output.push(track);
                if(track.children){
                    // No need to spread
                    // => output = this.mapRecursive(track.children)
                    // !!! Be aware you are reseting `output` !!!
                    output = [...this.mapRecursive(track.children)];
                }
            });
            return output;
        }
        this.state = {
            currentFolder: tracks,
            parentFolders: [],
            selected: tracks.children.filter(track => !track.children)[0],
            sideDrawer: false,
            favoriteTracks: props.match.path == "/favorite",
            importOpen: false,
            displayedItems: this.getListData(tracks,props.match.path == "/favorite"),
            playerRef: undefined,
            // All functions should be class methods
            onListClick: (track) => {
                console.log("onListClick");
                if(track === undefined){
                    this.state.navigateUp();
                }else if(track.children){
                    this.state.navigateToFolder(track);
                }else{
                    this.setState({selected: track});
                    this.state.restartPlayer();
                }
            },
            navigateToFolder: (track) =>{
                console.log("navigateToFolder");
                if(track.children){
                this.setState(state => ({currentFolder: track,parentFolders: [...state.parentFolders,state.currentFolder],displayedItems: this.getListData(track,false)}));
                }
            },
            navigateUp: () => {
                let parents = [...this.state.parentFolders];
                const newCurrent = parents.splice(parents.length-1, 1)[0];
                this.setState(state => ({currentFolder: newCurrent,parentFolders: parents,displayedItems: this.getListData(newCurrent,false)}));
            },
            toggleDrawer: (open) => {
                //if(this.state.sideDrawer !== open){

                    this.setState({sideDrawer: open});
                //}
            },
            onNextClick: () =>{
                const children = this.state.displayedItems;
                const index = children.indexOf(this.state.selected)+1;
                const boundaries = index === children.length ? 0: index;
                for(let newIndex = boundaries;newIndex < children.length;newIndex++){
                    if(!children[newIndex].children){
                        this.setState({selected: children[newIndex]});
                        this.state.restartPlayer();
                        return;
                    }
                }
            },
            onPrevClick: () =>{
                const children = this.state.displayedItems.filter(track => !track.children);
                const index = children.indexOf(this.state.selected) -1;
                const boundaries = index < 0 ? children.length-1: index;
                for(let newIndex = boundaries;newIndex >= 0;newIndex--){
                    if(!children[newIndex].children){
                        this.setState({selected: children[newIndex]});
                        this.state.restartPlayer();
                        return;
                    }
                }
            },
            setPlayerRef: (node) =>{
                // This is not invoked...
                // Not sure about the goal of this function, but
                // avoid storing HTML node ref into state
                // In your JSX, just : 
                // <div ref={node => this.playerRef = node} />
                // You'll then be allowed to access your class
                // Alternate new method (prefered):
                // this.playerRef = React.createRef()
                // // later...
                // <div ref={this.playerRef} />
                if(node != this.state.playerRef && node != null){
                    this.setState({playerRef: node});
                }
            },
            restartPlayer: () =>{
                if(this.state.playerRef != undefined){
                    this.state.playerRef.load();
                    this.state.playerRef.play();
                }
            },
            setFavoriteTracks: (value) =>{
                this.setState(state=>({favoriteTracks: value,displayedItems: this.getListData(state.currentFolder,value)}))
            },
            setImportOpen: (value) =>{
                this.setState({importOpen: value});
            },
            onListFavoriteClick: (track) =>{
                track.favorite = track.favorite ?!track.favorite: true;
                this.setState(state =>({displayedItems: this.getListData(state.currentFolder,state.favoriteTracks)}));
            },
        };
        //this.setState({displayedItems: ,});
    }

    render (){
        // Now I understand better why you are using state as much... 
        // Think before using context, there probably other way to do so
        // Newer versions of React let you use hooks, which are kind
        // of (reusable) helpers to add super powers to your components
        return (
            <PlaylistContext.Provider value={this.state}>
                {this.props.children}
            </PlaylistContext.Provider>
        );
    }
}

[rbenzazon]

Merci Raph pour la review :-) Pour le moment j'ai bossé en mode "ain't nobody got time for that" mais en sachant où je merdais pour revenir plus tard quand je suis moins noob. Même le theme/style est dégueu. je veux utiliser des vars mais je dois apprendre les themes mui.

Constructor : I had trouble making the context work. Yes I prefer removing it as soon as I make it work without this ugly state declaration. I swear the initial class looked better :-)

The spread in mapRecursive it's purposely but still shitty. I'll make a beautiful version.

Yes all functions are not where they are supposed to, like I said I'll work on the context structure.

setPlayerRef, this is ugly but the audio tag needs some bump(load(),play()) after refreshing src attr from the context when changing song. I could store in the component the previous src value and comparing it like a componentReceiveProps would do. I'll look into your advice.

I wanted a reusable state component, I tried hooks that didn't want to compile with class components. But I'll have a look.

Yesterday I refactored Playlist.js into Layout.js broken into separate classes. It taught me more about class structure. I now incrementally understand the concept of wrapping as well (I use withStyle with MuiThemeProvider and the context) so I can maybe clean it all up easily. I wanna try more TS as well. Check the simple search and the ./todo.txt I added. The search took me 2 hours so bear with me.

One problem I had with the context is the constructor / initial value building, the bad class structure, which forced me into gymnastics (method outside state obj that don't ever read state but args). The best solution would be a topological dependency structure between the described values, now it's a procedure inside the constructor/methods which is prone to errors. Another problem is when I update a prop from an object which is an item of a state prop array(favorite track), I think I'm forced to reassign the same state prop containing the displayed item to trigger the render. I don't know if there is a good way in react to update an item from an array which is a state prop without redrawing the whole list. I didn't check yet how bad the redraw is, maybe even when I type in the search bar the file list updates because of the setState({searchKeyword... I need to master the optimization of the redraw in react before delivering good apps.

[rbenzazon]

the ugly context class is now properly declared and cleaned, supports now several routes and doesn't blink each time the route is changed