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

feat(react): allow user provided reportError func #619

Merged

Conversation

StaberindeZA
Copy link
Contributor

@StaberindeZA StaberindeZA commented Jul 31, 2023

Because:

  • We should allow users the ability to provide a reportError function.

This commit:

  • Adds a class property, reportError, to ReactLocalization that allows
    consumers to provide a function on initialization to report errors.
    By default the errors will console.warn.

Closes: #411

Copy link
Member

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

When @stasm originally suggested using a provider prop for this, the code was structured differently, and that may have made more sense. Now, all of the error reporting calls are made from within ReactLocalization, so really the control should be via an argument of its constructor, rather than as a side effect of including it in a provider. Also, why make it a boolean rather than a function accepting an error argument?

@StaberindeZA StaberindeZA force-pushed the fluent-react-control-error-reporting branch from c5fee25 to e598e9d Compare August 1, 2023 12:38
@StaberindeZA
Copy link
Contributor Author

When @stasm originally suggested using a provider prop for this, the code was structured differently, and that may have made more sense. Now, all of the error reporting calls are made from within ReactLocalization, so really the control should be via an argument of its constructor, rather than as a side effect of including it in a provider. Also, why make it a boolean rather than a function accepting an error argument?

Thank you for the quick reply and comments. Hmm, I did not notice the age of the issue. 😅 Your suggestions make sense.

No specific reason for the boolean. For some reason when I initially read the issue, I missed some comments, and had in my mind that the ask was for an on/off toggle. Agreed though the function is better, and I've made the changes.

@StaberindeZA StaberindeZA changed the title feat(react): add switch to provider to log errors feat(react): allow user provided reportError func Aug 1, 2023
Copy link
Member

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

Much better! One small change, and this should be good to merge.

fluent-react/src/localization.ts Outdated Show resolved Hide resolved
Because:

* We should allow users the ability to provide a reportError function.

This commit:

* Adds a class property, reportError, to ReactLocalization that allows
  consumers to provide a function on initialization to report errors.
  By default the errors will console.warn.

Closes: projectfluent#411
@StaberindeZA StaberindeZA force-pushed the fluent-react-control-error-reporting branch from e598e9d to 26d6b59 Compare August 1, 2023 13:29
@eemeli eemeli merged commit 5f521c9 into projectfluent:main Aug 1, 2023
4 checks passed
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.

Control error reporting via a LocalizationProvider prop
2 participants