Update React development stack #72

watsonbox · 2020-11-09T08:29:59Z

The goal here is to update the project to a more modern React stack, brining several benefits:

Using create-react-app is an established standard development stack which should make contributing easier
Build step separates production and development environments (file structure, tests, performance)
More robust development toolchain including live reload, auto test runs, linting etc
Much improved testing framework (JEST)
Up to date versions of React, Font Awesome, JSZip and FileSaver.js
Sass for styling
Clear path to full ES6 / TypeScript

Out of Scope

I've deliberately omitted some things in this step, in order to keep the scope under control:

Bootstrap upgrade
Removal of jQuery
Migration to TypeScript
New features

npx create-react-app exportify --template typescript (https://create-react-app.dev/docs/adding-typescript/)

- JSZip - FileSaver.js - jQuery for legacy code support - Fontawesome - Sass - Bootstrap and Bootstrap React Keeping old version of Bootstrap for now (upgrade to come later)

- Yarn for dependency management - react-fontawesome for Font Awesome 5 icon components - TypeScript (more to come) - SASS support - JSZip upgrade Note: I've kept certain things like Bootstrap 3 and jQuery in order to make changes incrementally.

- Auth redirect flow - Playlist loading (snapshot) - Single playlist export - Export all playlists

tesnouf-drivy

Some minors comments/suggestions:

Some code could be more ES6+ friendly and React Components are a little bit legacy (Class Component instead of functional) and constructors are missing

I didn't focus on the logical side but this little project is neat !

tesnouf-drivy · 2020-11-09T12:52:09Z

src/components/Login.jsx

+import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"
+import { authorize } from "helpers"
+
+class Login extends React.Component {


Despite not being deprecated, the nowadays way to go for Components are Functional components especially if you have no state. It's simpler and easier to write/test

Okay noted, I may simplify these then 👍

tesnouf-drivy · 2020-11-09T12:53:41Z

src/components/PlaylistExporter.jsx

+import { apiCall } from "helpers"
+
+// Handles exporting a single playlist as a CSV file
+var PlaylistExporter = {


all your var should be replaced by let or const

Yep I think I'll clean up all the legacy code file by file once this is merged 👍

src/components/PlaylistExporter.jsx

tesnouf-drivy · 2020-11-09T13:07:23Z

src/components/PlaylistExporter.jsx

+      tracks.forEach(function(row, index){
+        let dataString = row.map(function (cell) { return '"' + String(cell).replace(/"/g, '""') + '"'; }).join(",");
+        csvContent += dataString + "\n";
+      });


When iterating on a loop to build something, you could use the reduce function

const csvContent = tracks.reduce( (acc, row) => { return `${acc}${row.map(cell => '"' + String(cell).replace(/"/g, '""') + '"'; }).join(","))}\n` }, '');

tesnouf-drivy · 2020-11-09T13:20:38Z

src/components/PlaylistTable.jsx

+  state = {
+    playlists: [],
+    playlistCount: 0,
+    likedSongsLimit: 0,
+    likedSongsCount: 0,
+    nextURL: null,
+    prevURL: null
+  }


should be defined in your constructor:
https://reactjs.org/docs/react-component.html#constructor

I read that if the constructor isn't required for anything else, this is cleaner e.g.:

https://dev.to/ascorbic/class-fields-are-coming-heres-what-that-means-for-react--3a87

tesnouf-drivy · 2020-11-09T13:22:37Z

src/helpers.jsx

+      window.location.href = window.location.href.split('#')[0] + '?rate_limit_message=true'
+    } else {
+      // Otherwise report the error so user can raise an issue
+      alert(jqXHR.responseText);


I guess it's a critical case...

Meh, again I think this should be improved at some point.

tesnouf-drivy · 2020-11-09T13:24:16Z

src/components/Paginator.jsx

+        </nav>
+      )
+    } else {
+      return <div>&nbsp;</div>


it's a little bit odd to return an empty div
Could return null do the job ? or at least return <div></div> ?

tesnouf-drivy · 2020-11-09T13:26:36Z

src/components/PlaylistTable.jsx

@@ -0,0 +1,139 @@
+import React from "react"
+import $ from "jquery" // TODO: Remove jQuery dependency


your comment say it all ;)

tesnouf-drivy · 2020-11-09T13:31:45Z

src/components/PlaylistTable.jsx

+                <th style={{width: "30px"}}></th>
+                <th>Name</th>
+                <th style={{width: "150px"}}>Owner</th>
+                <th style={{width: "100px"}}>Tracks</th>
+                <th style={{width: "120px"}}>Public?</th>
+                <th style={{width: "120px"}}>Collaborative?</th>
+                <th style={{width: "100px"}} className="text-right">


you should define your width using css classes.
Directly defining the style object is required when using variable / computing the value.
Aside of separating the visual from your logic, doing so won't break your Jest snapshots if you decide to change a column width

Very good point about the Jest snapshots! Indeed, something else to add to the cleanup list 👍

watsonbox · 2020-11-09T16:19:28Z

Thanks a lot for the comments @tesnouf-drivy 🙏

pavelkomarov · 2020-11-09T20:04:33Z

My PR gets rid of all jquery. Not sure how my React is compared to this PR; it is the first and only thing I've ever done in React. Honestly don't love React. Super verbose and annoying.

* Remove old project structure * Initialize project using Create React App npx create-react-app exportify --template typescript (https://create-react-app.dev/docs/adding-typescript/) * Add dependencies - JSZip - FileSaver.js - jQuery for legacy code support - Fontawesome - Sass - Bootstrap and Bootstrap React Keeping old version of Bootstrap for now (upgrade to come later) * Migrate Exportify to create-react-app stack - Yarn for dependency management - react-fontawesome for Font Awesome 5 icon components - TypeScript (more to come) - SASS support - JSZip upgrade Note: I've kept certain things like Bootstrap 3 and jQuery in order to make changes incrementally. * Add test suite - Auth redirect flow - Playlist loading (snapshot) - Single playlist export - Export all playlists * Add Bugsnag for error monitoring * Replace bind(this) with arrow functions * Use literals template for string concatenation * Update README.md for create-react-app development stack * Don't use Bugsnag in development environment

This reverts commit 9e31b7f.

* Remove old project structure * Initialize project using Create React App npx create-react-app exportify --template typescript (https://create-react-app.dev/docs/adding-typescript/) * Add dependencies - JSZip - FileSaver.js - jQuery for legacy code support - Fontawesome - Sass - Bootstrap and Bootstrap React Keeping old version of Bootstrap for now (upgrade to come later) * Migrate Exportify to create-react-app stack - Yarn for dependency management - react-fontawesome for Font Awesome 5 icon components - TypeScript (more to come) - SASS support - JSZip upgrade Note: I've kept certain things like Bootstrap 3 and jQuery in order to make changes incrementally. * Add test suite - Auth redirect flow - Playlist loading (snapshot) - Single playlist export - Export all playlists * Add Bugsnag for error monitoring * Replace bind(this) with arrow functions * Use literals template for string concatenation * Update README.md for create-react-app development stack * Don't use Bugsnag in development environment

#72 (comment) 😅

watsonbox added 4 commits November 9, 2020 09:24

Remove old project structure

116b76f

Initialize project using Create React App

6b021b1

npx create-react-app exportify --template typescript (https://create-react-app.dev/docs/adding-typescript/)

Add dependencies

bc1c3a2

- JSZip - FileSaver.js - jQuery for legacy code support - Fontawesome - Sass - Bootstrap and Bootstrap React Keeping old version of Bootstrap for now (upgrade to come later)

watsonbox self-assigned this Nov 9, 2020

watsonbox changed the title ~~Update React Stack~~ Update React stack Nov 9, 2020

watsonbox changed the title ~~Update React stack~~ Update React development stack Nov 9, 2020

Add test suite

cede159

- Auth redirect flow - Playlist loading (snapshot) - Single playlist export - Export all playlists

watsonbox force-pushed the v2 branch from b80401c to cede159 Compare November 9, 2020 08:49

watsonbox marked this pull request as draft November 9, 2020 08:50

watsonbox added this to the November 2020 Refresh milestone Nov 9, 2020

watsonbox added the enhancement label Nov 9, 2020

tesnouf-drivy reviewed Nov 9, 2020

View reviewed changes

watsonbox mentioned this pull request Nov 10, 2020

Pavel's changes #70

Closed

watsonbox added 4 commits November 10, 2020 09:29

Add Bugsnag for error monitoring

01583f9

Replace bind(this) with arrow functions

94ea057

Use literals template for string concatenation

093d925

Update README.md for create-react-app development stack

a35e8d9

watsonbox marked this pull request as ready for review November 10, 2020 09:04

Don't use Bugsnag in development environment

9a87193

watsonbox merged commit 85ff3e4 into master Nov 10, 2020

watsonbox added a commit that referenced this pull request Nov 10, 2020

Revert "Update React development stack (#72)"

5e7afba

This reverts commit 9e31b7f.

watsonbox deleted the v2 branch November 19, 2020 11:36

watsonbox mentioned this pull request Nov 22, 2020

Update exportify.js #24

Closed

watsonbox added a commit that referenced this pull request Aug 28, 2024

Move playlist column widths into stylesheets

5ada270

#72 (comment) 😅

		@@ -0,0 +1,139 @@
		import React from "react"
		import $ from "jquery" // TODO: Remove jQuery dependency

Update React development stack #72

Update React development stack #72

Uh oh!

Conversation

watsonbox commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Out of Scope

Uh oh!

tesnouf-drivy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watsonbox commented Nov 9, 2020

Uh oh!

pavelkomarov commented Nov 9, 2020

Uh oh!

Uh oh!

watsonbox commented Nov 9, 2020 •

edited

Loading