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

[cache] Reimplementation to use the new core caches #191

Merged
merged 10 commits into from
Dec 11, 2022

Conversation

florian-h05
Copy link
Contributor

@florian-h05 florian-h05 commented Dec 8, 2022

Fixes #184.

Reference openhab/openhab-core#2887.
Reference openhab/openhab-core#3204.

Description

This implements:

  • for the current methods of cache:
    • check if the new caches from core are available
    • if they are available, log a deprecation warning and use the shared cache from core instead of the addon‘s shared cache implementation so that we can remove SharedCache from the addon
    • if they are not available (means the library runs on an „old“ openHAB version), do nothing and keep the current behaviour
  • add a private property to cache to allow access to the private cache
  • add a shared property to cache to allow access to the shared cache

Unit test is updated to test the JSCache class instead of the exported methods of the cache namespace, because testing the exported methods for each case is quite complex and given the fact that the compatibility layer will be removed at some time, I decided to not take that effort.

Testing

The functionality of JSCache is tested by the Jest unit test, the exports are tested by me.

... while keeping backward compatibility with older openHAB versions.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05 florian-h05 added the enhancement New feature or request label Dec 8, 2022
@florian-h05 florian-h05 added this to the to be released milestone Dec 8, 2022
@florian-h05 florian-h05 changed the title [cache] Reimplementation to use the core caches [cache] Reimplementation to use the new core caches Dec 8, 2022
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

florian-h05 commented Dec 8, 2022

@digitaldan @jpg0 @rkoshak Can you please review? This is urgent, because we should get this merged before the sixth milestome and the feature freeze comes on Sunday evening.

You only need to review cache.js and the README changes, the rest is not so important.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Copy link
Contributor

@sfriedle sfriedle left a comment

Choose a reason for hiding this comment

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

LGTM.

cache.js Outdated Show resolved Hide resolved
Copy link
Contributor

@rkoshak rkoshak left a comment

Choose a reason for hiding this comment

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

LGTM assuming either we can't get at the ruleUID or filename or we add that to the log.

*/
const put = function (key, value) {
return cache.put(key, value);
const logDeprecationWarning = (funcName) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we can get the ruleUID or filename to log out as part of this warning? At first some users may get hundreds or these warnings and knowing which rules they are coming from can help in prioritization in dealing with them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By using console.warn instead of log.warn we are logging to the logger name of the script/rule, so for file-based script to org.openhab.automation.script.file.filename.js and for UI to org.openhab.automation.script.ui.ruleUid.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

florian-h05 commented Dec 10, 2022

@digitaldan @jpg0
If you want to give your review, please do it until this evening (+11 hours from now). After this, I'll merge to get this in before the feature freeze; I've already got two reviews so it is not unreviewed.

@jpg0
Copy link
Collaborator

jpg0 commented Dec 10, 2022

LGTM!

Copy link
Contributor

@digitaldan digitaldan left a comment

Choose a reason for hiding this comment

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

LGTM

@florian-h05 florian-h05 merged commit fd61b53 into openhab:main Dec 11, 2022
@florian-h05 florian-h05 deleted the cache-core-reimplementation branch December 11, 2022 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cache] Change implementation to the new access-tracking cache in core
5 participants