Refactor createRedirectWithUsername

[adityagarg06]

Progress on #2042

Changes:

• Used useSelector instead of connect to handle the state access.
• Removed the mapStateToProps function as no longer needed.
• The createRedirectWithUsername function is removed as the URL is directly passed as a prop to RedirectToUser .
• Replace createRedirectWithUsername with RedirectToUser in routing.
• Added Proptypes.

I have verified that this pull request:

• [x] has no linting errors (npm run lint)
• [x] has no test errors (npm run test)
• [x] is from a uniquely-named feature branch and is up to date with the develop branch.
• [x] is descriptively named and links to an issue number, i.e. Fixes #123

[lindapaiste]

Love it! This is good stuff and my feedback is that we can take the cleanup even further.

• Used useSelector instead of connect to handle the state access.
• Removed the mapStateToProps function as no longer needed.

Good change 👍. I would like to see us using more selector function vs defining selectors inline, as it adds a layer of separation between the component and the shape of the Redux state. I thought that we already had a selectCurrentUsername selector but I guess we don't. So maybe we should create one and use it here?

• The createRedirectWithUsername function is removed as the URL is directly passed as a prop to RedirectToUser .
• Replace createRedirectWithUsername with RedirectToUser in routing.

This syntax change (switching from an HOC to a component) is going to be really helpful if we ever upgrade to react-router v6.

• Added Proptypes.

It definitely should have PropTypes and I don't know why we're not getting any errors for that right now. This will fix one of the issues that came up when updating our eslint configs (#2220). 🔧

Other suggestions:

• Let's access the history variable from a react-router hook instead of using the global constant. const browserHistory = useHistory();
• The existing if (username == null) { return; } logic would be more clean if we handled the if in the opposite direction. That is, if (username) { browserHistory.replace(url.replace(':username', username)); }. It won't do anything if there's no username, but we don't need to explicitly return.

Side note:

I starting digging into the code and got confused by where we initiate the redirect to the login page if the user is not logged in. If I go to https://editor.p5js.org/sketches when not logged in the redirect is working properly, but I don't see that logic anywhere in the routing.

It's actually happening on the server: https://github.com/processing/p5.js-web-editor/blob/91e5b79069c7d38555082b6f25bd566b8dbf74af/server/routes/server.routes.js#L90-L96

And now I'm starting to think about the bigger picture, and wondering if we should move the redirect to username part to the server too? Like after writing all that about how we can improve this component, I'm now wondering if we actually should delete the whole thing. Instead of the res.send(renderIndex()); we would have res.redirect(/${req.user.username}/sketches); and then a whole bunch of front-end code can be deleted.

[raclim]

Thanks for your work on this! This ended up getting resolved by #3147. I'm really sorry that we couldn't get this in, but please feel free to check out our other issues!