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

add async option to Hook so setTimeout in console methods can be disabled #77

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

Conversation

joshuahhh
Copy link

Problem

Because console-feed runs console methods in setTimeout, it cannot capture changes happening synchronously between logs. For instance,

var arr = [];
console.log("before", arr);
arr.push(3);
console.log("after", arr);

will log:

before ▶︎ [3]
after ▶︎ [3]

I believe this setTimeout was added for performance reasons. I'm sure that's preferable for some use-cases, but for other use-cases, it is confusing and certainly not ideal. (For example: processing/p5.js-web-editor#1203 (comment).)

I believe this issue is the root cause of #23 & #66.

Possible solution

I don't know the details of the performance issues the setTimeout was added to mitigate. It's possible the setTimeout should simply be removed.

If not, then it would be great if this behavior could be customizable. In this PR, I add a new option called async to Hook. By default, async is true, reproducing the current behavior. But if async is set to false, the setTimeout is replaced with immediate execution of its contents.

Hook already has a single boolean argument to control encode. Having multiple boolean arguments results in less-readable code, so I replaced this with an options object. (For backwards-compatibility, a user can still just pass in a single boolean for encode.)

Please let me know what you think of this approach! I'd be happy to make changes depending on what you think is best.

Thanks a bunch!

@catarak
Copy link

catarak commented Feb 11, 2021

This is rad! Thank you for working on this fix ✨

@joshuahhh
Copy link
Author

Hi @samdenty! Have you had a chance to look at this? I'd much appreciate your feedback. Thanks!

@catarak
Copy link

catarak commented Sep 13, 2021

Just wanted to bump this PR 😄 It would be great to fix this! FYI this bug is reproducible on CodeSandbox and the p5.js Editor.

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