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

Do not overuse state #1

Open
ghost opened this issue May 15, 2019 · 2 comments
Open

Do not overuse state #1

ghost opened this issue May 15, 2019 · 2 comments

Comments

@ghost
Copy link

ghost commented May 15, 2019

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
Copy link
Owner

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
Copy link
Owner

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

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

No branches or pull requests

1 participant