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

STRF-9222 Replace/upgrade deprecated messageformat library #246

Merged
merged 1 commit into from
Jul 20, 2021

Conversation

jairo-bc
Copy link
Contributor

  1. Removed disablePluralKeyChecks option as it's not supported anymore
  2. "Zero" key is invalid for lang "en". (https://unicode-org.github.io/cldr-staging/charts/latest/supplemental/language_plural_rules.html)
    Before library was ignoring it, now it throws an error. In cornerstone we don't have it (https://github.com/bigcommerce/cornerstone/blob/master/lang/en.json), but other developers might have it.
  3. There is change in lang helper logic. Before it was throwing a SyntaxError if there is some "syntax error", now it always a generic Error. So the idea is to throw an error always when the value is invalid and log a message.

cc @bookernath, @bigcommerce/storefront-team

if (err.name === 'SyntaxError') {
this.logger.error(`Language File Syntax Error: ${err.message} for key "${key}"`, err.expected);

return () => '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Before we were returning, and now we're throwing.

Does this mean that on a production store, the page would return an error such as 500, instead of the helper simply failing quietly and returning an empty string?

@bookernath
Copy link
Contributor

bookernath commented Jul 15, 2021

Removed disablePluralKeyChecks option as it's not supported anymore

What are the implications of this for real themes?

"Zero" key is invalid for lang "en". (https://unicode-org.github.io/cldr-staging/charts/latest/supplemental/language_plural_rules.html)

Seems okay based on a quick check of our other themes as well. 👍

There is change in lang helper logic. Before it was throwing a SyntaxError if there is some "syntax error", now it always a generic Error. So the idea is to throw an error always when the value is invalid and log a message.

See my comment in the code; I want to make sure we're not causing pages to fail that were not failing before. Changing error/logging syntax is fine, but we shouldn't cause new full-page errors if we can avoid it. Apologies if I misunderstood the code and we're all good :)

@jairo-bc
Copy link
Contributor Author

@bookernath You were right with your assumption. With that change it was failing on storefront-renderer, but not on GraalVM.
So I made a change to return a function that returns an empty string (like it was before).

@jairo-bc jairo-bc force-pushed the STRF-9222 branch 2 times, most recently from 8bd208d to f7fec14 Compare July 16, 2021 16:21
@bookernath
Copy link
Contributor

Thanks, LGTM

@jairo-bc jairo-bc merged commit 90a84f5 into bigcommerce:master Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants