Blacklist UI implementation

[poltak]

Takes over from #84. This branch has been rebased onto master by @Treora and blacklist UI dev can continue from here.

[poltak]

@Treora to address your last reply in #84:

I would pull the main component out of index.jsx to keep things clear and separated

Fair enough. 4445276 address this (index.js now simply handles rexporting)

I am not certain about the choice between state.blacklist and state.settings.blacklist

Generally I think it's good to keep redux state as flat as possible, having top-level keys for each group of state considered a module. Then there's the next step of normalising redux state (normalizr is popular for this), but where the project is now I think it's over-engineering; maybe revisit in the future if UI state becomes more complex.

A related concern is that the src/options/ and src/overview/ have separate redux stores. I'm still fairly new to how web extensions are structured, so I'm assuming there is some kind of limitation here and these UIs need to have separate states? (I've noticed other differences, like browser API is not available in options UI, while it is in overview UI)

configurePersistence is specific to the blacklist module, so logically it should go in the blacklist folder.

Good point; didn't think of that but it is specific to this module and was introduced in this contribution. 2ed4b001fd8cb2b2d4786ee963145d4be2dd92e2 moves it to blacklist/persistence.js and exposed in the blacklist module entrypoint as named export configurePersistence.

It may not be worth the effort to modularise [configurePersistence] neatly however, because when we have multiple settings modules we will probably want to persist all settings using a single generic approach instead.

See if you think what I did in that commit (moving it inside the feature module and exposing it via entrypoint) is a good idea. It is code specific to the UI feature module, hence I think each module should implement it and expose the same interface if they need state persistence. Then UI feature module persistence can be called in one place just after the redux store is init'd.

Just to summarise the feature module refactoring commits, feature module's entrypoint now exports:

• default (main container of the feature module)
• reducer (combined reducer for the part of state related to this feature module; combine with other feature module reducers)
• actions (object containing all redux actions related to this feature module)
• constants (object containing any misc constants related to this feature module; redux state keys, local storage keys, etc.)
• configurePersistence (function which optionally configures needed state persistence with local storage/PouchDB)

What do you think of this as a standard UI feature module interface?

[poltak]

Forgot to reply to this:

If you leave [redux state flattening] like this, the reducer and actions files from the settings folder could be removed.

At the moment I've just removed all the blacklist actions + reducers from those files (basically everything - in this branch at least) and left them as empty shells.

Didn't want to remove them just yet as they might be being used by other contributors, and might be confusing for whoever resolves the conflicts later when they saw I removed them.

After writing that and actually looking at the other activity, it doesn't look like anyone's working on other stuff inside the settings dir (src/options/containers/settings); (apart from in #65, where only minor refactoring into container and view components has been done, which is fine) so yes, I might as well remove that and clean it up.

[Treora]

I'm assuming there is some kind of limitation here and these UIs need to have separate states?

Making two apps rather than a single-page app was a design choice by Aquib and myself:

• Firstly: they are two separate pages, opened through separate paths; why combine them in one app?
• Making modular Redux code is challenging, separating keeps things clearer and less complex.
• Less complexity makes it easier to develop in parallel by different people.
• The two apps don't appear to need any shared state, except the persisted state, which they share anyway.
• We thought we may want optimise the overview to be light-weight, but I doubt the significance of this argument.

(I've noticed other differences, like browser API is not available in options UI, while it is in overview UI)

Except in Firefox you need a polyfill, add this line to options.html: <script src="/lib/browser-polyfill.js"></script>.

See if you think what I did in that commit (moving it inside the feature module and exposing it via entrypoint) is a good idea.

Good idea indeed. If we are taking the neat approach though, I would consider to not expose it by this custom name configurePersistence, but more generically as an initialisation function for that module. Other modules could have other types of initialisation. The application would just call each module's initialisation function and pass it the store, so the module can do what it needs to. Redux already exposes a way to do abstract this: store enhancers. They can do even more than just initialising (they can wrap the whole store), but you can also just use them for this purpose. I just took this approach for the query-in-url synchronisation in 4bf29cf.

What do you think of this as a standard UI feature module interface?

So for one I would abstract configurePersistance to an enhancer. Besides that, exporting the React component as default seems somewhat okay, but I would rather keep it explicit. For comparison, the ducks structure uses default for the reducer, which seems equally understandable. I stayed closer to Jack Hsu's example: export default { actions, components, constants, reducer, selectors };

With the enhancer (and also epics) added, and with selectors deemed unnecessary to export, the module interface of the overview is defined as such: export default {reducer, actions, epics, enhancer, components: { Overview }}

The separation into two apps allows for doing things differently, but if you think this makes sense we could keep a consistent pattern.

[poltak]

@Treora Thanks for outlining the option/overview separation reasons; I understand that aspect of things better now. No disagreements.

So for one I would abstract configurePersistance to an enhancer

That makes sense. Will go about doing that. TBH, I didn't look into how you had structured the overview module.

... and with selectors deemed unnecessary to export...

Not sure if I agree with omitting selectors from the interface, but I'm not sure about the future intended features of options or overview modules and how much/if state will need to be shared between feature module containers.

With the components export, I'm a bit confused as the Jack Hsu article doesn't seem to make a distinction between view/dumb and container/smart components. I don't see any reason to expose view components (any common -- shared between feature modules -- view components could be in a separate place), so is the main purpose of this (in your mind) to expose the containers? I think it would be nice to design the feature modules to be succinct enough to only have to use a single container that can easily be dropped into the main overview/options JSX layout.

[Treora]

Not sure if I agree with omitting selectors from the interface, but I'm not sure about the future intended features of |options| or |overview| modules and how much/if state will need to be shared between feature module containers.

selectors can of course be exported if needed elsewhere.

With the |components| export, I'm a bit confused as the Jack Hsu article doesn't seem to make a distinction between view/dumb and container/smart components. I don't see any reason to expose view components (any common -- shared between feature modules -- view components could be in a separate place), so is the main purpose of this (in your mind) to expose the containers?

Only expose components that are intended for outside use. Probably that is only containers (I don't usually distinguish these so carefully; when using redux-react I suppose containers are just the connected components, while the stuff defined in jsx is the dumb view component)

[poltak]

@Treora Have a look at this now when you have time. Would be good to get sorted out and hopefully merged in as we discussed the other day.

Had a bit of a look over and prompted me to clean up some minor things to get it to pass the CI build. Also brought over a commit from our fork to fix the browser-polyfill include in options.html and move over the blacklist store enhancer from the chrome browser API. Rebased changes from (this repo's) master, which prompted me to migrate all the React.PropTypes usage to prop-types package as you've done (good stuff).

Mainly things we were discussing before we left off seem to be the UI feature module structure. I'm pretty happy with the current structure, but see what you think now since last time (I've also structured the imports UI in worldbrain in a similar way).

[Treora]

Thanks so much for updating these PRs! I will have a better look, try it out, and maybe tinker with it one of the next days.

[Treora]

Tried it out, seems to work; a few things I'd change:

• pull material-design-icons from npm
• styling: input (e.g. no italics), remove clear input button (or show only on hover), fixed box height with scrollbar only when needed.
• make overview and options exports consistent

I can take it up myself as you moved on to other things; I might leave it to wait a bit though, as I anyway consider quickly shipping a first version with only a manual saving button.

By the way, an old bug shows up again: when storage permission is enabled, for some mystical reason pouchdb does not receive change events for changes in other tabs, so the overview does not react to new visits. Perhaps not too big a problem now, but an annoying bug (already reported).

[poltak]

make overview and options exports consistent

By this, do you mean make the main index.js for the UI module export like this? If so, I still have a problem with putting everything on the default export: I just don't see the point when you have named exports to take advantage of (maybe I'm missing something?)

With the current build process, I don't think there is any difference in the final result between using named exports or putting everything on default in the context of the UI feature modules: If you try to grab something from that UI feature module, all the code is gonna get pulled into the final build. Although if you do move to something like rollup, which supports tree shaking, taking advantage of named exports will enable you to do things like import { constants } from 'blacklist' from non-UI places, like background script, without pulling in the entire feature module, and thus unwanted UI code.

That's just an example, but the main point I'm trying to say is that it seems silly to put everything on default when you can take advantage of named exports in the ES6 modules system, even if many build tools don't really differentiate them yet (again, maybe I'm missing something; JS module systems are confusing)

[Treora]

With 'making exports consistent' I am fine with changing either (or both) of them.

I just don't see the point when you have named exports to take advantage of

Me neither, actually! I think named exports is more logical indeed, I probably just copied the approach from this example. I would not use any default exports at all as nothing is the obvious 'main' item.

[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. :)