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

Revamped error handling #8

Closed
3 of 4 tasks
amannn opened this issue Dec 3, 2020 · 5 comments · Fixed by #13
Closed
3 of 4 tasks

Revamped error handling #8

amannn opened this issue Dec 3, 2020 · 5 comments · Fixed by #13
Labels
enhancement New feature or request

Comments

@amannn
Copy link
Owner

amannn commented Dec 3, 2020

As mentioned by @ilanbm in #6 currently we throw for missing messages.

After some research I can think of two use cases where you want to keep the app running even if messages are missing:

  1. You don't have the messages yet. They'll be provided by a translator later on and you'd like to have a functioning app during development. However a warning would be helpful here.
  2. During unit tests you might want to assert for message ids to avoid hardcoding labels.

In both occasions it would be good to print the error by default, but opting out might be helpful.

What we could do, is to add an optional onError property on the provider which will allow the user to decide if and how errors should be handled. This can for example be used to turn off warnings about missing messages in unit tests.

I think errors should have the following properties:

  • code: An identifier that the user can detect.
  • message: A human readable message. This is only provided during development to reduce bundle size in production.
  • level: An enum with ERROR or WARN keys but which correspond to integers to be able to do checks like error.level >= NextIntlErrorCode.WARN.

Generally I can think of three categories of errors currently:

  1. Configuration issues which will break the app (e.g. missing NextIntlProvider). This is an ERROR, but we can't use a user-defined onError in this particular example, as there's no provider.
  2. Formatting errors which the developer has to fix (e.g. a missing value of a message). These use the WARN level, as we can render the message instead.
  3. Missing messages which need a translator to fix. These use the WARN level and render the ID instead, as the developer might not be able to fix it.

The default onError handler should throw an error for ERROR and print a console error for WARN (only in development).

TODO:

  • Log missing messages errors to console instead of throwing.
  • Introduce structured error instances and configurable onError prop to cover useTranslations.
  • Decide about an API to customize what is returned when a message is missing (e.g. <NextIntlProvider getMessageFallback={({path, error, message}) => ''} />)
  • Decide if we want to add error handling to useIntl functions as well. These should typically only be triggered by missing APIs (due to missing polyfills) or if the native APIs are used in an invalid way. Maybe it's ok to simply use the native error handling here as it's the developers job to fix it and there's e.g. no translator necessary.
@amannn amannn added the enhancement New feature or request label Dec 3, 2020
@ilanbm
Copy link

ilanbm commented Dec 3, 2020

Nice work!

You don't have the messages yet. They'll be provided by a translator later on and you'd like to have a functioning app during development. However a warning would be helpful here.
During unit tests you might want to assert for message ids to avoid hardcoding labels.

It is more than that. You have also production running apps that are connected to translation services that affect production live (like i18next with locize.com), or deployed occasionally with new locales files that were not created by developers. Its a production issue, not dev mode - hence should be warning and not error.

...add an optional onError property

Maybe you meant to it but I guess onError should also have access to returning a fallback value (could be none, could be the key)
In a lot of projects, the key is actually the English text to be translated, in others it is not.
so if it is not a separate flag (fallback: key, fallback: none) then it should be handled somehow as an option in onError

@amannn
Copy link
Owner Author

amannn commented Dec 3, 2020

You have also production running apps that are connected to translation services that affect production live

True, good point. With next-intl you can theoretically fetch translations from a remote resource – I wasn't sure if that's a valid use case.

Maybe you meant to it but I guess onError should also have access to returning a fallback value (could be none, could be the key)
In a lot of projects, the key is actually the English text to be translated, in others it is not.
so if it is not a separate flag (fallback: key, fallback: none) then it should be handled somehow as an option in onError

Right. I was thinking about providing a useful built-in handling here, but I guess it's justified that a user might wants to customize this.

I think it might be necessary to have a separate API from onError though, as for errors that originate from other issues than non-existing messages, returning a fallback might not be useful.

I've added a bullet above. My first API draft is:

<NextIntlProvider getMessageFallback={({path}) => ''} />

By using a function we can provide as much context as we have and the user can decide case-by-case what fallback should be used.

Does that look reasonable to you?

@ilanbm
Copy link

ilanbm commented Dec 4, 2020

yep, seems legit

@amannn
Copy link
Owner Author

amannn commented Dec 4, 2020

@ilanbm I've just released the discussed error handling capabilities in 0.3. Let me know if it works as expected for you!

@ilanbm
Copy link

ilanbm commented Dec 8, 2020

@amannn Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants