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

Memoize lookup in parse is producing false positives #6

Closed
henrinormak opened this issue Dec 10, 2018 · 3 comments
Closed

Memoize lookup in parse is producing false positives #6

henrinormak opened this issue Dec 10, 2018 · 3 comments

Comments

@henrinormak
Copy link
Contributor

This issue relates to the use of memoization in this code: https://github.com/i18next/i18next-icu/blob/master/src/index.js#L47-L59

Namely, given two strings with the following keys, foo.bar.baz and foo.bar, if parse happens to run first for foo.bar.baz, the later lookup for foo.bar will produce a false positive (the check on line 54 is truthy, eventhough this particular key has not yet been memoized), which leads to an error at line 58 (fc.format is undefined and not a function).

Likely it would be enough to swap the simple truthy check with something a bit more accurate (such as !fc && typeof fc.format === 'fuction').

Until then, the only real workaround (without changing keys) is to disable memoization (which is an overhead). Please let me know if you need further details, I can file a PR if needed.

@henrinormak
Copy link
Contributor Author

I suppose the memoization could perhaps work differently as well, avoiding nesting and instead using the key as a concrete property (i.e changing the way the utils work), but as I lack the context I might be missing something on why the memoize memory needs to be a deep object. Performance-wise it might actually be better to have a flat object.

It also raises a question about keys that contain .format. in them, as those would potentially have a conflict with the memoized IntlMessageFormat object.

@jamuhl
Copy link
Member

jamuhl commented Dec 10, 2018

how can you have 2 keys foo.bar and foo.bar.baz existing side by side? Did you set keySeparator to false on init? If so yes...we need to fix this by changing that to replace the . in keys (as that is the fallback on that set/get) https://github.com/i18next/i18next-icu/blob/master/src/index.js#L52

fc = utils.getPath(this.mem, ${lng}.${ns}.${key.replace('.', '###')});

would you like to provide a PR?

@henrinormak
Copy link
Contributor Author

Yes, I have configured keySeparator: false in i18next.init, I'll open a PR to do the replace.

This issue was closed.
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

No branches or pull requests

2 participants