separate functionality into main and renderer

[blahah]

Currently most of the action happens in the electron renderer process.

This means that things like downloading and fs operations are competing with the UI for processing.

We should separate out the IO operations from the UI, and have IO stuff run in the main electron process with the UI only in the renderer. This means communicating between the two with IPC, and will allow us to browserify the bundles separately for the two processes (see #46).

[greggraf]

Here are some thoughts:

General Approach

1. Reorganize

   • separate the code between the render and main threads in the directory structure. It would be nice to have two sibling directories ( like renderer/ and main/ or ui/ and app/) but for now building out the code for the main thread in app/ and creating an app/lib directory seems least intrusive.
   • move app/client/lib/constants.js up to app/constants.js as it will be used across the application, and update the various references. Maybe app/client/lib/imgpath.js to app/imgpath.js too.

2. Refactor out the code that doesn't need to communicate across processes

   • refactor app/client/lib/setup.js and create a new file app/lib/setup.js and move initial setup code (creating directories, setting up the local database) that doesn't need to communicate with the render thread
   • move app/client/lib/datasource.js, app/client/lib/getdatasource.js, and app/client/lib/localcollection.js into app/lib/ as well as any attendant files like stream-obj-rename.js.
   • investigate moving app/client/lib/contentServer.js, app/client/lib/fs-collect-stats.js, app/client/lib/fs-readstreams-progress.js, app/client/lib/hyperdrive-sync-entry.js, up to app/lib

3. Refactor the code that needs to communicate across processes

   • start to look at things that require communication between the threads like updater.js, paper.js
   • start to work through the models

Incremental PRs

I also think its important to do this as more frequent, smaller Pull Requests as the work involves reorganizing files. Making lots of changes to the file system all at once can adversely effect keeping existing branches and work in sync.

Here are some initial incremental pull requests I might make towards this approach:

• [x] PR 1: move app/client/lib/constants.js and app/client/lib/imgpath.js and refactor references - https://github.com/codeforscience/sciencefair/commit/e7e4c584b9223470e4e0a53bef97d4deff6b39fa #128
• [ ] PR 2: initial refactor of app/lib/setup.js directory setup logic
• [ ] PR 3: moving app/client/lib/datasource.js and supporting files up to app/lib
• [ ] PR 4: moving app/client/lib/localcollection.js up to app/lib
• [ ] PR 5: moving app/client/lib/contentServer.js up to app/lib
• evaluate the best next steps to take.

Does this seem like a reasonable approach?

[blahah]

@greggraf thank you so much for taking the time to put this together. This is a sound plan.

The main thing I want to raise is that quite a lot of the functionality in the app is intended to be broken out into smaller modules - see #118. I hadn't previously documented this plan, so you had no way of knowing :)

I'm raising this because I think quite a few of the module tasks can be done in parallel with this issue, but that anything that will be touched by the main/renderer separation should not yet be modularised. Once this issue is complete, the scope of those units will be more clear and we can then easily break them out into stable modules.

Any thoughts?

And are you interested in executing the plan you've suggested? :)

[greggraf]

Totally interested.

So I am soaking in the module project and it seems like mostly I should just have it in mind as I go through the code. Not immediately actionable per se, right?

[blahah]

@greggraf awesome!

And yes, I think that's right. I might start breaking some things out that are definitely not going to be tied up in the main/renderer split, and that I need for other tools, but I'll first wait for the lib movement so as not to cause confusion

[greggraf]

@blahah, wonder if you have a comment.

I was looking and I think this does not work: https://github.com/codeforscience/sciencefair/blob/master/app/run.js#L66

    main.webContents.emit('quitting')

I think it should be

    main.webContents.send('quitting')

This change gets into the callback in setup.js but leads to further errors:

after.js:32 Uncaught TypeError: Expected a function
    at after (/Users/greggraf/Sites/sciencefair/app/node_modules/lodash/after.js:32:11)
    at module.exports (/Users/greggraf/Sites/sciencefair/app/client/lib/alldone.js:6:19)
    at Datasource.self.close.cb [as close] (/Users/greggraf/Sites/sciencefair/app/client/lib/datasource.js:353:18)
    at datasources.all.forEach.d (/Users/greggraf/Sites/sciencefair/app/client/lib/setup.js:22:36)
    at Array.forEach (native)
    at EventEmitter.require.ipcRenderer.on (/Users/greggraf/Sites/sciencefair/app/client/lib/setup.js:22:21)
    at emitOne (events.js:96:13)
    at EventEmitter.emit (events.js:188:7)

It appears that lodash.after() expects a function as the second argument, but when d.close() is called, we don't pass a callback.

[blahah]

@greggraf well spotted! Must have been tired the day I wrote that 😩.

For the close, it's fine to just use a noop like () => {} I think.

[greggraf]

This is a branch that modifies the logic around datasources to use IPC, so that operations (deleting, downloading, creating files, database interaction, generating metadata) can happen on the main thread.

https://github.com/greggraf/sciencefair/tree/create-ipc-channel-for-datasources https://github.com/codeforscience/sciencefair/compare/master...greggraf:create-ipc-channel-for-datasources

I have identified interactions with app/client/lib/getdatasource.js and/or app/client/lib/datasource.js and tried to recreate that logic in app/lib/datasourceschannel.js using the IPC pattern.

For the purposes of development and testing, I have wrapped the new logic in a conditional statement, activating the new code, only when the environmental variable FEATURE set to "ipc" and have tried to keep all existing logic in place if that condition is not met.

In addition, when that condition check is true, the dev dataroot become sciencefair_dev_ipc to isolate the data and filesystems in each case.

To start the application using the new logic use: $ FEATURE=ipc npm run dev

This is a work in progress.

I am requesting feedback to see if:

• the general approach makes sense
• if there is something I missed

[blahah]

@greggraf gonna check this out ASAP, but just to say thank you so much for persevering with this, it's a huge contribution.

[greggraf]

@blahah: Picking up on the comments around a841d2d

I have updated my branch with the latest from master. This is not a PR yet because the branch only uses IPC when the flag is present. I wanted to be able to view the old code and new code side by side, and in fact compare the functionality by starting and stopping the application with the flag present or absent.

If the approach is sound, I can remove the switches and turn this into a proper PR.

This branch focusses on functionality around Datasources, the next step would be to rework Paper.

I would love to see what direction you have taken and how it is similar or diverges from what is here.

Please see the comment above for the detailed description: https://github.com/codeforscience/sciencefair/issues/88#issuecomment-329881351