Implement cache eviction logic for moduleBank

[ZhangYiJiang]

Currently when modules are loaded, they are stored and persisted in the moduleBank reducer state (https://github.com/nusmodifications/nusmods/blob/master/www/src/js/reducers/moduleBank.js#L48) to improve load performance and for offline use. We currently use localStorage for all client-side persistence. It is in theory possible to fill up the space if the user keeps opening new modules, but in practice this almost never happens. That said, I recently noticed a quota exceeded error on Sentry which indicates that someone (or a bot) really did hit 5MiB, so we should consider implementing this.

We should be able to set a cap on the size of the cache (either by number of modules or the length of the JSON.stringifiy string). Cache eviction can be done using least recently used (LRU) logic, though that takes some additional work as we don't record when the cache was refreshed for each module. This information can also be used in the future to improve our data loading logic. An easier alternative is to do random evictions, though that's less user friendly.

[Cloud-r]

I would like to work on this issue, if possible. This is gonna be my first request so any help would be appreciated. Thanks

[ZhangYiJiang]

@raiden-x Sure. Just note that this issue is much simpler if you already have some experience with Redux. The instructions for setting up the project can be found here https://github.com/nusmodifications/nusmods/tree/master/www, and uou can talk to us on Telegram https://telegram.me/nusmods or drop a comment here if you have any issue

[Cloud-r]

I seem to be facing some issue when starting or building. It's stuck with webpack : wait untill bundle finished: /. Install dependencies gave me this warning ' stylelint-webpack-plugin@0.10.4" has incorrect peer dependency "stylelint@^8.0.0'. Not sure if this is the issue though. The dependencies were installed successfully though.

[ZhangYiJiang]

@raiden-x That's just the plugin not updated, it works fine with our version of Stylelint. Are you on Windows by any chance? I've had another report of this happening on Windows. Otherwise it could just be the first build being a bit slow. Subsequent builds should be faster.

[Cloud-r]

It seems to be a problem with FlowStatusWebpackPlugin. Maybe the flow server is not getting initialized properly for me. Removing this finishes the start in about 83 secs. And yea, i'm using windows. I did wait for about 8+ mins during the initial build with flow.

[ZhangYiJiang]

I've noticed Flow starting up really slowly after upgrading to 0.82, though it could also have came from elsewhere. Not having Flow typing is not a dealbreaker, and you can try starting the server independent from Webpack by running yarn flow. After the server starts type checking runs very quickly.

[Cloud-r]

Ah thanks for the help

[Cloud-r]

Working on this, helpful if i can get a couple of days to finish this. Thanks

[Cloud-r]

I'm working on this and i notices that each module that gets stored in the persistModuleBank in the local storage seem to take about 20KB each. So I thought i would the cap the maximum limit for the modules(excluding the module list and other persists) to 3MB. So that gives about 150 modules that are stored at max. So when the new module gets written into the state from the reducer, I'm thinking that i would do a check there to see if there are 150 modules present and if that was the case i would remove the LRU one. Other method that i thought of was to use transform in redux-persist to delete it when adding it to local storage. We could also add compress transform for module list so that more modules could be stored in the localstorage. Does this seem okish, since i don't have much experience working with react and redux Thanks

[ZhangYiJiang]

@raiden-x That sounds good. Compression should be unnecessary - we're just doing a cache here, so anything other than moduleList, venueList and the module data for modules already on the timetable are just a bonus. 100 modules @ 2MB is probably more than reasonable.

Implementing it as a redux-persist transform is an interesting idea, but if it gets too complicated then just implement it as a reducer. Try not to modify the shape of the state because that requires migrations, which are always a bit of a pain. Also the logic should never evict modules which are in the timetable.

[Cloud-r]

So I made changes in the reducers as u suggested. Is it possible for the changes to get reviewed before I start writing up test cases so that I can implement any changes/improvements too if needed. Thanks

[ZhangYiJiang]

Sure, just open a PR and add "[WIP]" to the title. We won't merge PRs with that in the title.