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

Move CACHES import into function scope to prevent side effects. #9561

Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Jul 14, 2022

Summary

  • The default cache module involves module level access to the Kolibri OPTIONS object
  • This results in evaluation and initialization of the plugin registry
  • This causes an issue for anything importing from main expecting to be able to enable plugins, as the enabled plugins will not be properly registered

References

Fixes a reported issue for the Mac app.
I am hopeful that this behaviour would raise a runtime error in develop due to some stricter checking, but it might still need some follow up to prevent this happening in future.

Reviewer guidance

Test the mac app that is built from the WHL file from this PR and ensure it starts.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Jul 14, 2022
@rtibbles rtibbles added this to the Planned Patch 4: Coach usability improvements milestone Jul 14, 2022
Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

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

No doubt, thanks. I think I was who introduced the problem :(

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

@pcenov also confirmed that it installs and runs on the Mac device, we are good to go! 💯 :shipit:

@rtibbles rtibbles merged commit 296ced9 into learningequality:release-v0.15.x Jul 15, 2022
@rtibbles rtibbles deleted the cache_me_if_you_can branch July 15, 2022 14:43
@rtibbles
Copy link
Member Author

I think I was who introduced the problem :(

Yes - but the fact that nothing shouts at you if an import like this is added is the real problem here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants