Skip to content

Conversation

@morwoen
Copy link
Contributor

@morwoen morwoen commented Aug 4, 2020

Fixes #69

What happened

At the Localisation & Globalisation team at my company, we've decided to release an internal "test" locale (we called it xx-XX) to provide pseudo localisation for all other teams across. Since this locale is fictional there is no CLDR data for it. Our approach was to take es-ES and alter the data so that it acts as the new xx-XX locale.

During the process we've missed the likelySubtags object, meaning there was a mismatch

"main": {
  "xx-XX": {...}
},
"supplemental": {
  "likelySubtags": {
    "und": "es",
    "es-ES": "es"
  }
}

The error

Passing this data into a Globalize.load() (which in turn calls CLDR.load()) causes no errors, however, instantiating a new instance new Globalize(locale) throws TypeError: Cannot read property '0' of undefined. Note that what locale is above doesn't matter. Locales with already loaded valid data can no longer be used.

Expected outcome

An error to be thrown only once and the data is ejected from the queue to allow the application to continue without a need to restart.

Impact

The service that observed the error is using Globalize in its backend. This means that after an internal user (xx-XX is only an internal locale, no end-user can request it) tested the new locale end-users using actual locales started seeing errors due to the recurring exception thrown for valid locales.

Why this fixed it

I have moved the clearing of the _availableBundleMapQueue to before the iteration over it. That way even if there is an error in the code below the error will happen only once. There appear to be no other exit points before the queue clearing, so pulling it earlier is not changing the behaviour.

@rxaviers
Copy link
Owner

rxaviers commented Aug 4, 2020

Thank you for reporting the issue and for providing a solution in this PR. I have left one comment above. Thanks

@morwoen
Copy link
Contributor Author

morwoen commented Aug 5, 2020

Hi @rxaviers ,
I've updated the PR around what we discussed. It now mutates the array as it iterates over it so that when an error is thrown the problematic bundle will already be removed from the queue. I've added a more descriptive error message, feel free to suggest improvement to the language used.

I've also added some more comments to the test so it is easier to understand in the future for new people and once we forget about this :)

(Also added a signed-off-by to the commits)

@rxaviers
Copy link
Owner

rxaviers commented Aug 5, 2020

Awesome! Looks great. Thanks a lot

@rxaviers rxaviers merged commit b996f05 into rxaviers:master Aug 5, 2020
@morwoen morwoen deleted the fix-global-queue branch August 5, 2020 11:58
minBundle = minBundle.join(Cldr.localeSep);
existing = availableBundleMap[minBundle];
if (existing && existing.length < bundle.length) {
return;
Copy link
Owner

Choose a reason for hiding this comment

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

Late review... This should also be break.

rxaviers added a commit that referenced this pull request Aug 5, 2020
rxaviers added a commit that referenced this pull request Aug 5, 2020
rxaviers added a commit that referenced this pull request Aug 5, 2020
rxaviers added a commit that referenced this pull request Aug 5, 2020
@rxaviers
Copy link
Owner

rxaviers commented Aug 5, 2020

Amended by 2bac803

PS: In addition to the s/return/break mentioned above, there's something else that was causing regressions in PayPal libs I couldn't debug (time-wise). Amended using code closest to what we had before while keeping what @morwoen needed (please let me know otherwise). Thx

@morwoen
Copy link
Contributor Author

morwoen commented Aug 6, 2020

Ah, Completely missed that return :) yeah, I think it still achieves what we aimed for, thanks for validating it with your internal libs 🥇

rxaviers added a commit that referenced this pull request Oct 22, 2020
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.

Global state pollution bug

2 participants