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

[API CHANGE] Add optional graceful handling of missing variables #39

Closed

Conversation

Frooxius
Copy link
Contributor

@Frooxius Frooxius commented Nov 6, 2023

In application I am working on, we have a lot of user generated content which gets updated and changed over time.

This results in some of the configured variables to get out of date. This makes the formatting functions throw lots of exceptions, which litters the logs and it's not quite desirable.

I have added an optional boolean argument, which allows the formatting to quietly ignore missing variables where desired and just assume they're default values, which is more preferable behavior for my use-case.

NOTE: This includes commits from my other two PR's, since I had to do those to be able to compile and run tests.

@jeffijoe
Copy link
Owner

jeffijoe commented Nov 6, 2023

Hey @Frooxius , thanks for the PR, especially those locale-specific unit test fixes! 🙌

I don't think we should add a new parameter to FormatMessage for this. I do have an idea though. How about we allow passing in a custom FormatterLibrary, and make a change to FormatterLibrary's constructor to accept a allowMissingVariables boolean (false by default), as well as all the built-in formatters to accept this new configuration option? We would need test cases that cover the new behavior, of course.

What do you think?

@jeffijoe
Copy link
Owner

I closed this one by mistake, these changes did not make it in. However, I did find that FormatJS which is another implementation of MessageFormat also throws when missing variables, so I'll leave it closed for now.

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