Blacklist page blocking implementation

[poltak]

Using the blacklist data stored in local storage, this implements the actual page blocking logic in both import-history and activity-logger's log-page-visit modules.

Both these modules use the isWorthRemembering function to decide whether a given page should be remembered. This function has been extended to allow a further condition of checking for a URL match in the blacklist data (currently done very naively using String.prototype.includes()).

Originally implemented via accessing local storage inside isWorthRemembering, however seems to be a lot simpler if we just extend isWorthRemembering's interface to allow optional passing in of blacklist data (isWorthRemembering called n times for inputs that can grow really big for cases like importing history).

Some things that may not be welcome, that I realised looking over this:

• I changed the isWorthRemembering signature from accepting an object containing url key to two separate args url and blacklist; give me the signal and i'll revert that seemingly-arbitrary decision
• wrote a simple utility async wrapper around Array.prototype.filter(); was used in original implementation but not needed anymore. Just left it in for possible use later but can just remove it if it's not wanted

[Treora]

Nice. Thoughts:

• isWorthRemembering could now use a better name. shouldBeRemembered perhaps?
• I understand the desire to cache the parsed json, but the logger or import functions themselves should not have to know how to read the list from localStorage. Possible options, from easy to effort&pretty:
  • At least put the repeated code in a function, e.g. loadBlacklist(): Object
  • A functional approach, e.g.: const shouldBeRemembered = checkWithBlacklist(); shouldBeRemembered(url) (better name needed)
  • Hiding the whole caching inside the check function, ditching the cache on a storage change event.
• I wonder if code for reading and perhaps applying the blacklist should be in options/blacklist.
• Async wrappers may be handy some time, but let's not keep them in the code base while not used. We have version control to find things back if needed.

[poltak]

@Treora 5306f72 removes the blacklist local storage access to a HOF that handles that stuff and returns a version of shouldBeRemembered with blacklist data bound. If called without blacklist data bound, that condition is skipped. I went with this way as I'm not too sure exactly what you mean by:

Hiding the whole caching inside the check function, ditching the cache on a storage change event.

Can revise more if this way is deemed better.

I wonder if code for reading and perhaps applying the blacklist should be in options/blacklist.

At the moment everything in this feature module is just related to UI. Adding this seems out-of-place unless we add a utils (or something) named export to the module interface (what we're discussing in #93) to contain util fns that are not UI-related but feature-related (blacklist in this case). Maybe?

At the moment the blacklist local storage access, and blacklist-URL checking, logic is in src/activity-logger/index.js. It only lives here now because that's where the original isWorthRemembering fn was and this logic extended from the workflow invoked from that. But yeah, before considering all this to be merged in, it should find a more appropriate home (either somewhere in src/activity-logger or src/options/blacklist modules, I think).

[Treora]

@Treora https://github.com/Treora 5306f72 https://github.com/WebMemex/webmemex-extension/commit/5306f723b7aa338da6fa1c20d5f7524fc59bcba7 removes the blacklist local storage access to a HOF that handles that stuff and returns a version of |shouldBeRemembered| with blacklist data bound. If called without blacklist data bound, that condition is skipped. I went with this way as I'm not too sure exactly what you mean by:

Hiding the whole caching inside the check function, ditching the
cache on a storage change event.

Can revise more if this way is deemed better.

That's fine, the cache thing was a bit more complicated than needed. I would not bind the function to this, it creates less obvious code than passing arguments. I would simply define a returned function inside the other; perhaps something like return (args) => shouldBeRemembered({...args, blacklist}). I don't even think we would want to run the check without blacklist, so could also inline the whole shouldBeRemembered function as a closure.

I wonder if code for reading and perhaps applying the blacklist
should be in options/blacklist.

At the moment everything in this feature module is just related to UI. Adding this seems out-of-place unless we add a |utils| (or something) named export to the module interface (what we're discussing in #93 https://github.com/WebMemex/webmemex-extension/pull/93) to contain util fns that are not UI-related but feature-related (blacklist in this case). Maybe?

[edit] Forgot to answer this question. Not sure if it's the best place either indeed; would we want src/blacklist? Feels to small for that. Rather than a utils, I would then put UI in options/blacklist/ui or so. No particular preference now though.

[poltak]

@Treora this one also had a bit of a clean up and rebased in the changes from blacklist branch (the base for this PR). Nothing much has changed as it seems to work fine, it's just that question of where the blacklist fetching from local storage logic should go. It is very small and may be weird in its own src/blacklist module, but it still doesn't fit into the blacklist UI feature module. Still I think putting it in a src/blacklist/index.js is probably the safest option, unless you have any better suggestion now (talking about fetchBlacklistFromStorage fn from activity-logger, in case you've forgotten).

[Treora]

I suppose we could indeed put it in src/blacklist, and maybe move it src/config/blacklist when we get more configurable features like this that need to be written and read from the whole application.

I may still generalise at least the wording a bit, as I want to provide more flexibility for users, to e.g. stop logging altogether and only store pages that one marks manually (I think this would be a sensible behaviour at least in the first release).

[Treora]

Closing this old PR. As project focus has changed, the logging-all-pages feature is off the roadmap for the foreseeable future. Thanks again for the effort though. :)