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.

Sign up for GitHub

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

Jump to bottom

fix app crashing when reloads overlap #46416

Closed
wants to merge 5 commits into from
Closed

fix app crashing when reloads overlap #46416

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ace3c2d
throttle reload command
coado Sep 10, 2024
54a5e17
added flow types
coado Sep 11, 2024
ac95f70
Update packages/community-cli-plugin/src/commands/start/attachKeyHand…
coado Sep 17, 2024
46745e9
change throttle to debounce
coado Sep 17, 2024
bcd17e8
debounce to throttle
coado Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions packages/community-cli-plugin/src/commands/start/attachKeyHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,18 @@ import fetch from 'node-fetch';

const CTRL_C = '\u0003';
const CTRL_D = '\u0004';
const reloadTimeout = 700;
coado marked this conversation as resolved.
Show resolved Hide resolved

const throttle = (callback: () => void, timeout: number) => {
let previousCall = 0;
return () => {
const currentCall = new Date().getTime();
if (currentCall - previousCall > timeout) {
previousCall = currentCall;
callback();
}
};
};

export default function attachKeyHandlers({
cliConfig,
Expand All @@ -41,11 +53,15 @@ export default function attachKeyHandlers({
env: {FORCE_COLOR: chalk.supportsColor ? 'true' : 'false'},
};

const reload = throttle(() => {
logger.info('Reloading connected app(s)...');
messageSocket.broadcast('reload', null);
}, reloadTimeout);

Copy link
Member

@huntie huntie Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we switch this behaviour out for debounce? Reasoning:

  • I reckon the expected user behaviour, more often than not, is to observe an initially requested reload complete within the ~500ms from keypress — and then retry if they made an extra change. In most cases, I imagine that throttling and emitting a second reload could be unexpected/would interrupt landing on the reloaded app (unwanted behaviour).
    • Additionally, Fast Refresh should push an update to the app immediately after a reload, if code was changed during this small interval.
  • With this change, I'd also like to shorten the timeout to a flat 500ms — this guards against accidental rapid user keypresses, rather than aiming to synchronise with the end-to-end app reload time (which is variable). (As you've stated, this is a workaround rather than a fix to the underlying issue.)

Note

Sidenote: It might also make sense to apply the debounce operation to all keys, OR tie each command to the completion of each event via Metro's reporter — but leave this with me, I should be working in this file later this week.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At first I thought that throttling might have a better user experience as it performs the user initial operation immediately, but now I can see that it may lead to some unwanted behaviour. Thank you for letting me know! I've pushed the commit that changes the throttle to debounce.

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, I'm being a terrible front end engineer. For some reason I hallucinated that throttle would re-fire a second keypress 🙈. So long as subsequent calls are cancelled, then leading debounce behaviour (i.e. throttle) is better UX, you're right 👍🏻

Let's change back, and accept my eternal thanks? 🥹

const onPress = async (key: string) => {
switch (key.toLowerCase()) {
case 'r':
logger.info('Reloading connected app(s)...');
messageSocket.broadcast('reload', null);
reload();
break;
case 'd':
logger.info('Opening Dev Menu...');
Expand Down
Loading