prevent prop drilling in beamlineAction components

[walesch-yan]

This PR is a continuation of the work initialized in #1585 . It mainly focuses on removing props drilling for BeamlineActions components in an addition to a few smaller adjustments.

Small adjustments:

• Remove BeamlineActionsContainer.css file. It only consisted of one class, that was not used in the code.
• Remove useCallback for functions except handleOnPlotDisplay (here it was needed as otherwise re-renders would enter an endless loop. To fix this a refactoring of Plot1D.jsx is needed, but this would leave the scope of this PR)
• Replacing ternary operators with null on the false branch by && operations
• key of messages was chopped to only contain the timestamp
• a DEFAULT_DIALOG_POSITION constant was added to BeamlineActionsDialog

Prop drilling Reducement:

• moving functions (hideOutput and newPlotDisplayed ) previously defined BeamlineActionsContainer down into BeamlineActionsDialog (where they are actually getting called)
• stopBeamlineActions and startBeamlineActions are now dispatched where needed and not in container functions that is passed through the tree
• BeamlineActionsContainer takes actionsList directly from state
• BeamlineActionsContainer no longer synchronizes on state changes to the currentAction. Instead child component do this where needed, reducing the props exchanged between components to a minimum (0 for most)
• BeamlineActionsContainer previously had a function attribute called startAction this function did 3 different things before dispatching startBeamlineAction:
  - Check if action is in actionlist
  - filter arguments of List type actions
  - update internal plotIDByAction state. A dictionary that keeps track of all plotIds for every action (probably needs to be revisited during the above mentioned refactoring of Plot1D)
  This logic has been moved into the startBeamlineAction thunk instead of being passed down the component tree. This also meant the addition of plotIDByAction into the state, including necessary reducer and action

[walesch-yan]

Seems like cypress could not connect to firefox? worked after re-running the action

[axelboc]

Fantastic stuff!

The only thing I kinda disagree with is moving plotIdByAction into the global state. I understand you've done it to be able to move some of the logic to the thunk in order to reduce prop drilling even more. While the implementation is great, I will always recommend keeping the state as local as possible over any other consideration.

Here, the state belongs to a very narrow branch of the React tree, and it doesn't need to be persisted server-side. So I reckon it should really remain a local state in BeamlineActionsContainer.

[walesch-yan]

The only thing I kinda disagree with is moving plotIdByAction into the global state. I understand you've done it to be able to move some of the logic to the thunk in order to reduce prop drilling even more. While the implementation is great, I will always recommend keeping the state as local as possible over any other consideration.

Here, the state belongs to a very narrow branch of the React tree, and it doesn't need to be persisted server-side. So I reckon it should really remain a local state in BeamlineActionsContainer.

I see I was hesitant as well and I'm happy to change this back. Maybe we can take another look at this in a later stage again when Plot1D component will be refactored.

[walesch-yan]

I reverted the movement of plotIdByAction into the global state, these are the changes to compared to the initial commit:

• no new reducer, state change or action change
• startAction and updatePlotIDByAction functions back in BeamlineActionContainer, both are getting passed through the tree
• BeamlineActionsContainer synchronizes with the name of currentBeamlineAction form the global state. (but does not re-render on state changes of the current action, i.e. action starts running or is aborted)

[walesch-yan]

I kept this If statement I added in the additional commit, which is not on the develop branch, it basically prevents a lot of if cases in the child components and enforces a clear usage of this function to ensure that all the filtering steps and local state updates are done in the same place.

[axelboc]

Yeah, I get you, it's this condition, right? If I'm not mistaken, this condition was not present in the BeamlineActionDialog (nor in BeamlineActionForm and AnnotatedBeamlineActionForm), which was probably a bug?

It might be simpler to have an early return for the List case and to duplicate the dispatch:

setPlotIdByAction((prev) => ({ ...prev, [currentActionName]: null }));

if (cmd.argument_type === 'List') { // not sure if checking for `Object.keys(params).length === 0` is needed?
  const cmdArgs = cmd.arguments.map((arg) =>
    arg.type === 'float' ? Number.parseFloat(arg.value) : arg.value,
  );

  dispatch(startBeamlineAction(cmdName, cmdArgs, showOutput));
  return;
}

dispatch(startBeamlineAction(cmdName, params, showOutput));

[axelboc]

I think it's great given the number of props we're left with 👏 — I don't think there's a way to remove this handleStartAction prop completely anyway (because of setPlotIdByAction)

There's probably potential to go further, like finding a way to dispatch startBeamlineAction from the child components while still keeping the setPlotIdByAction call in BeamlineActionsContainer (and without duplicating logic, perhaps by having a dedicated Redux action to start "List" commands)... but it's not worth the effort IMO.

Huge improvement as is 💯

[axelboc]

Yeah, I get you, it's this condition, right? If I'm not mistaken, this condition was not present in the BeamlineActionDialog (nor in BeamlineActionForm and AnnotatedBeamlineActionForm), which was probably a bug?

It might be simpler to have an early return for the List case and to duplicate the dispatch:

setPlotIdByAction((prev) => ({ ...prev, [currentActionName]: null }));

if (cmd.argument_type === 'List') { // not sure if checking for `Object.keys(params).length === 0` is needed?
  const cmdArgs = cmd.arguments.map((arg) =>
    arg.type === 'float' ? Number.parseFloat(arg.value) : arg.value,
  );

  dispatch(startBeamlineAction(cmdName, cmdArgs, showOutput));
  return;
}

dispatch(startBeamlineAction(cmdName, params, showOutput));

[walesch-yan]

Yeah, I get you, it's this condition, right? If I'm not mistaken, this condition was not present in the BeamlineActionDialog (nor in BeamlineActionForm and AnnotatedBeamlineActionForm), which was probably a bug?

BeamlineActionDialog passed down two functions instead of having the if condition, then it passed startAction to BeamlineActionForm which is the form that is called for List actions and the dispatch to AnnotatedBeamlineActionForm. So this behavior is the same as having the condition check.

It might be simpler to have an early return for the List case and to duplicate the dispatch:

I see, so the length check I added was to have the possibility to pass your own parameters, which is probably not needed. I think the other issue is more that plotIdByAction was not updated for schema actions previously. (At least from my understanding this was a problem).