React class to functional conversion - final

[nahbee10]

Fixes #2709

Changes:
applied react functional component conversion to CollectionList, SketchList, FileNode, ConsoleInput, AssetList. This would be the last conversion ticket

I have verified that this pull request:

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

[release-com]

Release Environments

This Environment is provided by Release, learn more!
To see the status of the Environment click on Environment Status below.

• p5.js-web-editor
  • app-tedc896.release.editor.p5js.org/
  • preview-tedc896.release.editor.p5js.org/

🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-1672cfe859

[khanniie]

not done reviewing yet but figured i should submit the first wave of comments so you can get started!

[khanniie]

so i was looking into useCallback since that function is new to me and I read in the docs that it's typically used when functions are passed as props to prevent unnecessary rerenders -- I was wondering if we want to apply it to all of these functions that are being passed as onClick props? Or what's the logic for only using it for a few of them?

[nahbee10]

yeah @khanniie some of them don't have to be rendered using useCallback since:

• When to use useCallback:

1. Passing callbacks to child components: If a callback is passed as a prop to a child component and that child uses React.memo to prevent re-renders, using useCallback ensures that the callback doesn't get recreated on each parent re-render, preventing unnecessary child re-renders.
2. Expensive computations: If the function involves expensive computations (e.g., heavy calculations or side effects), wrapping it in useCallback can avoid recreating the function unnecessarily.

• Don't have to use useCallback:

1. No performance gain: For simple state updates or inline event handlers (e.g., handleChange, handleClick), memoization won't provide much benefit and adds unnecessary complexity.
2. Not passed as props or dependencies: If the function isn’t passed as a prop or used in a useEffect dependency array, memoizing it is unnecessary.

[raclim]

Thanks so much for working on this! It's really awesome that all of these files are getting revamped!

I'm doing some quick manual testing here, and I couldn't find any major issues in most of the changed components except for FileNode.jsx, which seems to cause some crashes when opening certain example sketches (i.e 3D: shader as a texture). I wanted to also start pointing this out now, but will continue to review more closely!

[raclim]

Just took a closer look through some of the files! Added a few more thoughts, let me know if you have any questions about any of them, thanks!

[raclim]

Just wondering if you intended on keeping this for this PR!

[nahbee10]

Thanks for the review and fix @raclim and @khanniie !! Just updated code and requested review again :)

Seems like the error from filenode is solved after fixing the button onclick function @raclim!!

[khanniie]

i feel like since this is from the constructor we probably don't need anything in the dependency array?

[lindapaiste]

dispatch never changes its value so it doesn't make a difference whether it's in the dependency array or not. The effect will run one time on mount either way. Personally I'm in the habit of including it to appease linting rules such as react-hooks/exhaustive-deps. But we don't have that rule enabled on this repo (at least not yet) because we violate it all the time.

[khanniie]

same here, since its from the constructor do we need the dependency array?

[lindapaiste]

If the value of propsUsername were to change then we would want to redo the fetch with the new username. So you need that as a dependency.

If the projectId were to change then you would want to redo the project fetch, but not the collection fetch. So I would have multiple useEffect hooks with separate dependencies.

  const username = propsUsername || user.username;
  
  useEffect(() => {
    if (projectId) {
      getProject(projectId);
    }
  }, [projectId]);
  
  useEffect(() => {  
    getCollections(username);
  }, [username]);
  
 useEffect(() => {
   resetSorting();
 }, []);

I'm not sure what you mean by "from the constructor"? In practice, with the way that the app is set up, the value of propsUsername probably doesn't change without a total unmount and remount because we never link from one user profile to another. Maybe if you've viewing another user's collection page and you click the collections link from your user dropdown? That's the sort of situation where there could be bugs if we don't have the right dependencies.

[khanniie]

thank you nahee!!!!

[raclim]

Thanks so much for your work on this @nahbee10!

I think maybe one additional change that would be great is to remove the console.log in the client/modules/IDE/components/Console.jsx file if that wasn't needed for a specific purpose! Otherwise, I think this is good to go!

[raclim]

Thanks so much again @nahbee10 and @khanniie! I'm going to merge this in! Feel free to make any additional changes if anything comes up!

[lindapaiste]

It seems like the username should be required and it's a mistake that we don't have it marked as such. I'm basing that off of two observations:

1. The current code does not have any sort of fallback for if the username is not provided in the getCollections() call.
2. The current condition in getTitle for checking if this is your own collection or someone else's is if (this.props.username === this.props.user.username) {. So that definitely implies that when viewing your own collection we would expect that username prop is explicitly provided and that it matches the username of the current logged-in user.

[lindapaiste]

Not a big deal but FYI it's better to have separate useSelector calls for each piece of state that you read:

Call useSelector Multiple Times in Function Components

When retrieving data using the useSelector hook, prefer calling useSelector many times and retrieving smaller amounts of data, instead of having a single larger useSelector call that returns multiple results in an object. Unlike mapState, useSelector is not required to return an object, and having selectors read smaller values means it is less likely that a given state change will cause this component to render.

However, try to find an appropriate balance of granularity. If a single component does need all fields in a slice of the state , just write one useSelector that returns that whole slice instead of separate selectors for each individual field.

https://redux.js.org/style-guide/#call-useselector-multiple-times-in-function-components