[Port] Localization: fix bugs around inconsistent locale + first step to centralize locale resolution #3340
Conversation
| @@ -123,6 +123,8 @@ describe('LGLanguageGenerator', function() { | |||
|
|
|||
| this.beforeAll(async function() { | |||
| const multiLanguageResources = await LanguageResourceLoader.groupByLocale(resourceExplorer); | |||
|
|
|||
| //Should have a setup for the threadLocale here. | |||
There was a problem hiding this comment.
| } | ||
|
|
||
| const locale = this.context.activity?.locale; | ||
| if (locale !== undefined) { |
There was a problem hiding this comment.
in .NET, here is
if (!string.IsNullOrEmpty(locale)). if we keep the same here, it will break some tests in LGGenerator.test.js
Do we have a workaround to change the enviroment locale in node?
There was a problem hiding this comment.
How is the environment locale set in .NET? Since we don't have threads or any other natural isolation, setting values on a context parameter (i.e. this.context) is likely the cleanest way to achieve the same functionality.
There was a problem hiding this comment.
In .NET, it is
Thread.CurrentThread.CurrentCulture = new CultureInfo("es-ar");Or maybe we can slightly modify the tests in javascript since it is hard to modify the environment locale.
There was a problem hiding this comment.
I guess I'm wondering if this is something we can really do in Javascript. There is no notion of threads or domains (well, they're deprecated anyways) to support collecting context or culture. We have functions and arguments, and we already have a context argument that gets passed around. Is that context parameter not sufficient?
There was a problem hiding this comment.
I think the current workaround is OK. An empty string in locale, both LG and expression will fallback to the correct locale resolution.
Pull Request Test Coverage Report for Build 636945958
💛 - Coveralls |
libraries/botbuilder-dialogs-adaptive-testing/tests/multiLanguageGeneratorTests.test.js
Outdated
Show resolved
Hide resolved
libraries/botbuilder-dialogs-adaptive/src/input/confirmInput.ts
Outdated
Show resolved
Hide resolved
libraries/botbuilder-dialogs-adaptive/src/recognizers/multiLanguageRecognizer.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| const locale = this.context.activity?.locale; | ||
| if (locale !== undefined) { |
There was a problem hiding this comment.
How is the environment locale set in .NET? Since we don't have threads or any other natural isolation, setting values on a context parameter (i.e. this.context) is likely the cleanest way to achieve the same functionality.
| @@ -12,7 +12,7 @@ import { DialogContext } from '../../dialogContext'; | |||
| /** | |||
| * @private | |||
| */ | |||
| const TURN_STATE = Symbol('turn'); | |||
| const TURN_STATE = 'turn'; | |||
There was a problem hiding this comment.
@stevengum, the only issue I could see here is conflicting with code that used the 'turn' state key. Is that me being too paranoid about backward compatibility?
There was a problem hiding this comment.
We do have some built in turn states with string keys, is there a way to notify our users that these keys are reserved keys?
There was a problem hiding this comment.
Not that I know of - @stevengum any thoughts?
closes: #3311 #3188
Also includes changes in microsoft/botbuilder-dotnet#5019.