Skip to content

Error on invalid dispose function. #162

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

Merged
merged 5 commits into from
Mar 17, 2020
Merged

Conversation

mbostock
Copy link
Member

Fixes #89.

My initial inclination was to just log a warning, but since warnings are only visible if you open the developer console, I figured that wouldn’t actually help people recognize the mistake.

@mbostock mbostock requested a review from visnup March 15, 2020 17:16
@visnup
Copy link
Member

visnup commented Mar 16, 2020

For the example linked in #89 this would instead immediately error for that cell with "invalid dispose"? Is it possible to give a hint to the mechanism for the error in the message?

@mbostock
Copy link
Member Author

Saying that the returned dispose function is invalid is a hint, no?

We could have an error message that specifically targets promises by looking for a then-able if you think that’s worth the trouble.

@visnup
Copy link
Member

visnup commented Mar 17, 2020

Re-reading the README docs on Generators.observe which does document the dispose return function, so it could be enough, but I don't think it's obvious that the return value is always consumed that way, especially when mixing with async.

I was thinking of the warning you get if you try to use an async function inside of useEffect:

@mbostock mbostock merged commit b833ab0 into master Mar 17, 2020
@mbostock mbostock deleted the warn-on-invalid-dispose branch March 17, 2020 17:09
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.

Generators.observe and Generator.queue throws a TypeError on the console (side effect)
2 participants