-
Notifications
You must be signed in to change notification settings - Fork 96
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
Issue-297: Language of GVL not being cloned #306
Issue-297: Language of GVL not being cloned #306
Conversation
…o cloned, updated unit tests.
modules/core/src/GVL.ts
Outdated
if (this.lang_ !== GVL.DEFAULT_LANGUAGE) { | ||
|
||
/* | ||
* changeLanguage is an asynchronous function but always behaves synchronously when called from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is true. What if the purposes-[LANG]
is in the process of being fetched? Seems like we'd want to .then()
on that promise, and we'd then be asynchronous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and there's actually a timing problem above, on #L428 , where we set lang_
before we've received the response)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shortaflip and I have had a discussion, and I think the answer is this clone()
needs to be documented that it is only properly synchronous after a changeLanguage
if the changeLanguage
has been await
-ed on. Otherwise, if a changeLanguage
is still loading the new purposes file, the clone()
will cause another purposes fetch to start and the new cloned GVL won't be fully ready until that returns (but for which there is no indication, since clone()
cannot be async or return a Promise).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@a2intl
I checked, I'll explain the flow, the clone() function on the TCModel is called when the "custom" consent is applied by the user. I only launch the UI layer after loading the language (so after loading and awaiting the changeLanguage()), and I call clone() only after a user action and a user can only interact when the UI layer is loaded and displayed in the browser.
So I think the problem is not calling clone() only after running changeLanguage(). Because my code already does what you say.
Here a possible more appropriate solution perhaps:
If you want I can also tell you the problem, when you go to make the clone, a new instance of the GVL is created, if you check the lang_ property inside the constructor it is set to default, at the end after having populated the new GVL instance, calls the cacheLanguage() method with this.lang_, perhaps the least "invasive" solution would be to make a variable static (lastLanguageApplied) with the last language value, at the beginning, the default could easily be the same as the default lang_ but then it must be changed, and then use in the constructor the static variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marco-prontera, thanks, I was probably unclear: we need to BOTH apply this PR (which fixes your bug by properly applying the copied-from GVL's language) AND document the caveat that clone()
may have asynchronous effects unless the last changeLanguage
has been (properly) awaited on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be some potential timing issues with setting the language vs. cloning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved, but we should add some documentation that "changeLanguage must be awaited before calling clone".
…g a GVL with a non-default language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
…ssue-297 Issue-297: Language of GVL not being cloned
Closes #297