close
The Wayback Machine - https://web.archive.org/web/20220514144829/https://github.com/bikeshaving/crank/issues/186
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

More error logging #186

Closed
brainkim opened this issue Jan 15, 2021 · 2 comments
Closed

More error logging #186

brainkim opened this issue Jan 15, 2021 · 2 comments
Labels
enhancement

Comments

@brainkim
Copy link
Member

@brainkim brainkim commented Jan 15, 2021

I think having Crank emit errors/warnings in more situations would go a long ways towards compensating for the docs' lack of nuance.

E.g., an async generator returning undefined doesn't strike me as a common thing to want to do, and if someone wants it maybe they should return null so it's explicit. Then Crank could throw an error if an async generator returns undefined. That should points the user in the right direction to debug things.

Or maybe if the user calls this.schedule() and the component doesn't yield for X seconds, emit a warning. That'd catch my mistake where I thought the refresh should get scheduled because the code executed the this.schedule line.

I'll bet there's some low-hanging fruit to make some assumptions about user intent a bit more explicit, and sprinkle throw / console.warn() around inside Crank when it has a hunch the user's not doing what they think they're asking for.

Originally posted by @spiffytech in #185 (comment)

@brainkim
Copy link
Member Author

@brainkim brainkim commented Jan 15, 2021

My thoughts:

  • Warn if components yield/return undefined.
    Probably.
    This is probably a good idea, insofar as it would catch a lot of implicit returns, especially in generator component functions which accidentally exit. And the technique of using explicit nulls is a nice way to squash this error. I continue to hate the fact that JavaScript has two nothing types (undefined and null), but maybe we can take advantage of this feature for this case.
  • Warn if generator components return early.
    Probably not.
    Do we want to log here or are there valid use cases for returning early? I’m holding out for potential use-cases where returning from a generator component is useful with keys to describe fixed async sequences that happen for a specific prop, but it’s something to think about.
  • Warn if arrays/iterables have unkeyed children.
    Probably not.
    There are lots of valid use-cases for unkeyed elements in arrays, and for React, all this warning ever did was make people key elements by index. This kind of warning also perpetuated the idea that keys were exclusively for collections of elements, whereas it actually has many more use-cases like forcing a detail page to rerender on page transition.
  • Warn if schedule() is called and the component doesn't yield for X seconds.
    Probably not.
    Not sure about this one, insofar as I feel like there’s probably gonna be a lot of false positives for long-running components.

I’d like to only include warnings which have a near perfect hit rate in terms of something the developer shouldn’t be doing, or those which provide a way to squelch the error like making implicit returns explicit with null. The problem is that developers treat these warnings like hard errors and will get upset if, for instance, a dependency causes errors to be logged.

One other constraint is that we’re not going to be doing build shenanigans to strip errors in production. If it’s good enough to log in development, it’s good enough to log in production, and I really do not want any divergent behaviors between the two environments.

@brainkim brainkim added enhancement good first issue labels Jan 15, 2021
@brainkim
Copy link
Member Author

@brainkim brainkim commented Oct 8, 2021

In 0.4 we now warn when undefined is yielded or returned. Other error logging requests should probably go in new issues.

@brainkim brainkim closed this Oct 8, 2021
@brainkim brainkim removed the good first issue label Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement
Projects
None yet
Development

No branches or pull requests

1 participant