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

pull up cleanup functions #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

pull up cleanup functions #1

wants to merge 1 commit into from

Conversation

rkuhn
Copy link
Member

@rkuhn rkuhn commented May 5, 2024

No description provided.

Copy link
Member Author

@rkuhn rkuhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the change is basically to switch from useState to useRef and manual re-render, right?

return next
})
const del = <T extends Record<string, unknown>>(sync: () => unknown, map: Map<string, Mgmt<T>>, id: string) => {
map.delete(id)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not enough: when stopping to follow a plant or robot, the machine-runner needs to be destroyed right here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see, the missing cancel call

if (effectHasRun) return
// eslint-disable-next-line react-hooks/exhaustive-deps
else effectHasRun = true
let alive = true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed? shouldn’t it be enough to track cancellation callbacks and calling them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed so that runPlant and runRobot know when the useEffect's lifetime ends.
The alternative to this will be a sync runPlant and runRobot, as you told me the last time, returning a cancellation callback which when called will flip a similar flag, although the alive flag will be inside each of runPlant and runRobot

@@ -146,22 +157,36 @@ export const App = ({ actyx }: Props) => {
myPlant = persistentId('plantId')
console.log('restarting: plant', myPlant)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to update the plantId state as well.

if (!old) return
const readyState = { ...old, type: 'ready' as const, state };
map.set(id, readyState);
sync()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be conditional on the state actually being different

@rkuhn
Copy link
Member Author

rkuhn commented May 5, 2024

@Kelerchian
Copy link
Contributor

Kelerchian commented May 6, 2024

So the change is basically to switch from useState to useRef and manual re-render, right?

The main change is the switch from Mgmt[] to Map<Mgmt> to get a free duplication check, and the cancellations.
The change from useState to useRef is so that the syntax in add, del etc is not confusing because we're dealing with a Map.
Otherwise, we will be forced to do something like this, which makes the code unnecessarily complex:

setter((oldMap) => {
  const newMap = new Map(oldMap) // must always be done because React's state comparison uses equality and the same reference to Map will always be true and will skip a render trigger
  // operate on newMap here...
  return newMap
})

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

Successfully merging this pull request may close these issues.

2 participants