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

Handled invalid formatter function props being passed #212

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Bluebberies
Copy link
Contributor

Problems

  1. When the formatter prop is provided as a value that is not a function (e.g., null, undefined, or a non-callable object), the component raises an exception indicating that "formatter is not a function." This results in a stack trace being logged, which exposes the internal error and disrupts the user experience.

  2. When a valid function is provided to the formatter prop, if that function throws an error during execution, it leads to an unhandled exception that crashes the application. This prevents the TimeAgo component from rendering as intended, impacting the overall functionality and user interface.

Solutions

  1. Implemented a check to determine if the formatter is a function. If it is not, a warning is logged, and the component falls back to using the defaultFormatter. This ensures that the component can still render valid output without crashing.

  2. Wrapped the call to the formatter in a try-catch block to catch any errors that may arise during its execution. If an error is thrown, a warning is logged, and the component gracefully falls back to the defaultFormatter.

Ensured all test cases pass

Copy link
Owner

@nmn nmn left a comment

Choose a reason for hiding this comment

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

The fix makes sense, but there are some mistakes that need to be fixed.

Thanks!


try {
// Call the formatter to see if it behaves correctly
const result = formatter(value, unit, suffix, then, nextFormatter, now)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const result = formatter(value, unit, suffix, then, nextFormatter, now)
const nextFormatter = defaultFormatter.bind(null, value, unit, suffix)
const result = formatter(value, unit, suffix, then, nextFormatter, now)


const nextFormatter = (value, unit, suffix) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const nextFormatter = (value, unit, suffix) => {
const formatter = (value, unit, suffix) => {

return (
<Komponent {...spreadProps} title={passDownTitle}>
{formatter(value, unit, suffix, then, nextFormatter, now)}
{/* {formatter(value, unit, suffix, then, nextFormatter, now)} */}
{nextFormatter(value, unit, suffix)}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
{nextFormatter(value, unit, suffix)}
{formatter(value, unit, suffix)}

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