Electron build of JBrowse desktop #520

garrettjstevens · 2019-09-24T20:15:17Z

This started out as a bit of a crazy idea, but now I kind of hope it works out. Basically this makes a new create-react-app (CRA) app for jbrowse-sv-inspector-desktop and builds it in electron. The development version works fine, and I'm working on getting it to build a production version.

As I was working on getting a new electron project going, I tried to start from scratch, but there were so many webpack, babel, etc. things to set up. There are some boilerplates out there, but none of them were great. Really, nothing as nice as what CRA put in place for us in jbrowse-web.

Then I found this article on creating an electron app using CRA. It's not exactly what we want, but has some good ideas and gave me a good starting point. Currently you can try out yarn start in jbrowse-sv-inspector-desktop and it runs basically a copy of jbrowse-web in an electron window.

Things still to see:

Can we get good cross-platform builds out of this? (The article above uses electron-builder, which I'm experimenting with now)
Can we integrate the electron-specific features we want (file system access, etc.) in a safe way using this?

This article above also uses something called rescripts, which lets you hack your CRA setup without ejecting. I actually used it to un-eject jbrowse-web, so that both it and jbrowse-sv-inspector-desktop can take advantage of the latest CRA updates and we don't have to maintain the dev and build systems separately.

resolves #195
resolves #484
resolves #512

make run in electron

garrettjstevens · 2019-09-29T01:16:34Z

Brain dump from WIP:

Webpack "electron-renderer" target assumes nodeIntegration is true, which used to be the default for electron, but now is not. We want to avoid nodeIntegration for security reasons.
Tried to create a custom target. The target worked, but if a target isn't one of webpack's pre-defined ones, you lose a whole bunch of presets and some npm modules won't work because they think you're in a node environment.
Using "web" target for now, which works, it just doesn't take advantage of "Externals" in webpack, so it will probably make the bundled size bigger.
We want to use ipc calls to communicate between renderer and main processes. Renderer (which does not have access to node) sends an ipc call to Main (which has node access), Main then e.g. runs fs.open and returns the filehandle to the Renderer.
With nodeIntegration off, you can't import electron, which you need in order to do renderer<->main thread ipc calls.
Workaround is to use a "preload" process that imports electron and assigns it to a global variable.
Web workers can't see the global electron, though, and they're the ones that really need it since they do the file access.
There is a nodeIntegrationInWorker setting where you can enable node just in webworkers. This would be great, except compiling webpack with a "web" target clobbers the native node modules. __non_webpack_require__ does not seem to work.
Would be best to do webpack on the workers separately with the "webworker" target (or actually maybe "node" target?), but not sure yet if that works with this setup.
Using MainThreadRendering, I was able to get an example of the renderer<->main thread RPC calls mostly working.
For some reason, getting uncaught errors with an empty error object (no explanation, name, anything). Might be related to aborting.

rbuels · 2019-09-30T04:10:32Z

Dude thanks so much for this deep dive. So many dragons in there!

…

On Sat, Sep 28, 2019 at 6:16 PM Garrett Stevens ***@***.***> wrote: Brain dump from WIP: - Webpack "electron-renderer" target assumes nodeIntegration is true, which used to be the default for electron, but now is not. We want to avoid nodeIntegration for security reasons. - Tried to create a custom target. The target worked, but if a target isn't one of webpack's pre-defined ones, you lose a whole bunch of presets and some npm modules won't work because they think you're in a node environment. - Using "web" target for now, which works, it just doesn't take advantage of "Externals" in webpack, so it will probably make the bundled size bigger. - We want to use ipc calls to communicate between renderer and main processes. Renderer (which does not have access to node) sends an ipc call to Main (which has node access), Main then e.g. runs fs.open and returns the filehandle to the Renderer. - With nodeIntegration off, you can't import electron, which you need in order to do renderer<->main thread ipc calls. - Workaround is to use a "preload" process that imports electron and assigns it to a global variable. - Web workers can't see the global electron, though, and they're the ones that really need it since they do the file access. - There is a nodeIntegrationInWorker setting where you can enable node just in webworkers. This would be great, except compiling webpack with a "web" target clobbers the native node modules. __non_webpack_require__ does not seem to work. - Would be best to do webpack on the workers separately with the "webworker" target (or actually maybe "node" target?), but not sure yet if that works with this setup. - Using MainThreadRendering, I was able to get an example of the renderer<->main thread RPC calls mostly working. - For some reason, getting uncaught errors with an empty error object (no explanation, name, anything). Might be related to aborting. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#520?email_source=notifications&email_token=AAASAFM24OXOPSST4OY53UDQL76XJA5CNFSM4I2ELE2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD73FOVA#issuecomment-536237908>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAASAFJC5222IRDMM2YPB53QL76XJANCNFSM4I2ELE2A> .

garrettjstevens · 2019-10-01T00:53:28Z

Error in the last bullet point above was not due to aborting, it just appeared that way at first. It was actually due to errors in the main process. It turns out errors that originate in the main process don't contain any information when passed to the renderer process. To see main process errors, you have to try/catch them and log the error, and then it will show up in the terminal window that electron is running from (won't show up in dev tools).

With GMOD/bam-js#42 incorporated, I now have a working version using a local BAM file and MainThreadRpcDriver.

codecov · 2019-10-06T01:53:36Z

Codecov Report

Merging #520 into master will decrease coverage by 5.83%.
The diff coverage is 14.68%.

@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
- Coverage   66.62%   60.79%   -5.84%     
==========================================
  Files         247      266      +19     
  Lines        8417     9300     +883     
  Branches     1930     2109     +179     
==========================================
+ Hits         5608     5654      +46     
- Misses       2591     3373     +782     
- Partials      218      273      +55

Impacted Files	Coverage Δ
packages/alignments/src/CramAdapter/CramAdapter.ts	`61.17% <ø> (ø)`	⬆️
...ckages/sequence/src/TwoBitAdapter/TwoBitAdapter.ts	`100% <ø> (ø)`	⬆️
...lignments/src/BamAdapter/BamSlightlyLazyFeature.ts	`67.02% <ø> (ø)`	⬆️
...c/StructuralVariantChordRenderer/ReactComponent.js	`94% <ø> (ø)`	⬆️
packages/core/util/layouts/GranularRectLayout.ts	`87% <ø> (+1%)`	⬆️
packages/jbrowse-web/src/workerPolyfill.js	`16.66% <ø> (ø)`	⬆️
packages/circular-view/src/index.js	`100% <ø> (ø)`	⬆️
packages/bed/src/BigBedAdapter/BigBedAdapter.ts	`89.28% <ø> (ø)`	⬆️
...ackages/variants/src/VcfTabixAdapter/VcfFeature.ts	`56.66% <ø> (ø)`	⬆️
.../config/src/FromConfigAdapter/FromConfigAdapter.ts	`100% <ø> (ø)`	⬆️
... and 87 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1babf91...8e64e7b. Read the comment docs.

Also get rid of unused pluginManager in worker driver constructor

garrettjstevens · 2019-10-13T01:18:09Z

CORS-busting is working, dependent on GMOD/generic-filehandle#42. This is the gnomAD SV VCF, which doesn't work in jb-web because of CORS:

Once that PR is merged I'll mark this ready for review.

…loping on laptop+desktop setups

Add new empty session card and placeholder new SV Inspector card

garrettjstevens · 2019-10-30T04:21:55Z

Start screen is working now, so I think this is ready for review.

cmdcolin · 2019-10-30T18:33:13Z

I added a couple comments.

Some other random ones

Would be good to get some sort of automated testing of the electron app. JBrowse 1 used spectron to do this and it actually works on travis-CI.

Also sometimes the app shuts down without warning and in the console it says "Maximum number of clients reached". Wondering if it's related to the screenshoting logic (maybe the screenshot is a client? not sure)

[1] [7673:1030/143057.530126:ERROR:CONSOLE(24)] "Empty response arrived for script 'devtools://devtools/remote/serve_file/@74afbfc65bd4d271bc09280abd5783ffe47444b9/product_registry_impl/product_registry_i
mpl_module.js'", source: devtools://devtools/bundled/shell.js (24)
[1] [7673:1030/143057.530200:ERROR:CONSOLE(109)] "Uncaught (in promise) Error: Could not instantiate: ProductRegistryImpl.Registry", source: devtools://devtools/bundled/shell.js (109)
[1] [7673:1030/143057.530212:ERROR:CONSOLE(109)] "Uncaught (in promise) Error: Could not instantiate: ProductRegistryImpl.Registry", source: devtools://devtools/bundled/shell.js (109)
[1] [7673:1030/143057.530218:ERROR:CONSOLE(109)] "Uncaught (in promise) Error: Could not instantiate: ProductRegistryImpl.Registry", source: devtools://devtools/bundled/shell.js (109)
[1] tracing_service_impl.cc: Configured tracing, #sources:2, duration:0 ms, #buffers:1, total buffer size:102400 KB, total sessions:1
[1] Maximum number of clients reached[7673:1030/143102.488372:ERROR:shared_x_display.cc(37)] Unable to open display
[1] wait-on http://localhost:3000 && electron . exited with code 0

packages/jbrowse-web/public/index.html

cmdcolin · 2019-10-30T18:16:01Z

packages/jbrowse-web/tsconfig.json

@@ -9,14 +9,15 @@
    "allowJs": true,
    "skipLibCheck": true,
    "esModuleInterop": true,
+    "allowSyntheticDefaultImports": true,


I think it may be better to not use this flag, not sure though

I'm not sure either, but I will say that this is the default tsconfig for a fresh typescript-enable CRA.

cmdcolin · 2019-10-30T18:16:11Z

packages/jbrowse-web/tsconfig.json

@@ -1,6 +1,6 @@
 {
  "compilerOptions": {
-    "target": "esnext",
+    "target": "es5",


Might as well keep it as esnext?

cmdcolin · 2019-10-30T18:17:18Z

packages/linear-genome-view/src/LinearGenomeView/index.ts

@@ -90,41 +90,41 @@ export function stateModelFactory(pluginManager: any) {
      configuration: types.frozen(),
    })
    .views(self => ({
-      get viewingRegionWidth() {
+      get viewingRegionWidth(): number {


In general, I like having automatically determined function return types, if there is a rule requiring them I would prefer to keep it off

packages/wiggle/src/BigWigAdapter/BigWigAdapter.ts

cmdcolin · 2019-10-30T18:21:25Z

packages/jbrowse-desktop/src/ui/App.js

+  session: PropTypes.observableObject.isRequired,
+}
+
+export default withSize()(observer(App))


The App.js from jbrowse-web does not use withSize. We may want a way to de-duplicate logic between jbrowse-web and jbrowse-desktop in some cases, not sure what the right approach is though

cmdcolin · 2019-10-30T18:29:02Z

packages/jbrowse-desktop/public/test_data/config.json

@@ -0,0 +1,1372 @@
+{


I'd like this config.json to be the same as JBrowse desktop so I can compare all tracks

packages/jbrowse-desktop/public/electron.js

garrettjstevens added 7 commits September 23, 2019 14:12

Initial simple electron app shell

f5b36ac

Initial CRA run

82336f7

Use rescripts to un-eject CRA

a7c6c12

Update eslint rules, fix a lot of lint errors

4ed9eda

Use node 10 in CI

a0fbab2

Add back accidentally delected action

29470e5

Copy jbrowse-web to jb-sv-inspector-desktop

b9779df

make run in electron

garrettjstevens added 2 commits September 25, 2019 13:35

Add back webpack-cli

b61b077

WIP

0aeefd3

garrettjstevens added 10 commits October 1, 2019 12:37

More electron WIP

33c7e2b

Initial working electron with local file via IPC

a0b619e

Delete a file and fix the lockfile

60221a1

Make ElectronLocalFile not crash jbrowse-web

b8adc2b

Remove redundant tests and update snapshots

8e324be

Use app.getAppPath() instead of __dirName

0c25d44

More strictly emulate fs.promises.read() behavior

b629c82

Make jbrowse-web production build work again

75f1b0d

Update deps

447c2bb

Get rid of worker stuff in jb-desktop

f0242c5

rbuels assigned rbuels and garrettjstevens and unassigned rbuels Oct 7, 2019

garrettjstevens added 3 commits October 9, 2019 14:59

Log the block error so you can see a stack trace

0b53659

Properly pass timeout to rpc's call

23e873e

Also get rid of unused pluginManager in worker driver constructor

Move where block error is logged from

d72089e

garrettjstevens added 4 commits October 10, 2019 20:20

Send canvas data over electron IPC as dataURL

ac572de

Remove duplicated error log

58fedfe

Terminate window workers on page reload

30979be

Factor out BaseRpcDriver class

55c6ee2

garrettjstevens and others added 14 commits October 25, 2019 15:45

Add ability to desktop app to view non-cors-enabled files

2f6d315

Update rangeFetcher TypeScript

ea98564

Add basic start screen with recent sessions

f3d5155

accept a DEV_SERVER_URL environment variable for electron.js for deve…

4273713

…loping on laptop+desktop setups

Outline for a new session area in start screen

7f31bb9

Add new linear genome view of dataset option to start screen

d8df1c6

Make window bigger, don't show menu bar, fix worker path name

065d8a2

Use config template if no config found on startup

54648d0

Update to new gnomAD SV VCF

0e0704c

Factor our new session cards

7ff8b45

Add new empty session card and placeholder new SV Inspector card

Make a "back to welcome screen" entry in file menu

7eb935c

Add "factory reset" to advanced settings

9f56fe4

jbrowse-sv-inspector-desktop -> jbrowse-desktop

f569ab7

Merge branch 'master' into cra_rescripts

4def34d

garrettjstevens marked this pull request as ready for review October 30, 2019 04:19

rbuels changed the title ~~Electron build of JBrowse (to become sv-inspector)~~ Electron build of JBrowse desktop Oct 30, 2019

cmdcolin reviewed Oct 30, 2019

View reviewed changes

garrettjstevens added 3 commits October 30, 2019 15:15

Fix title and description in index.html

e053747

Simplify with fs rename and path extname, basename

593588a

Add attribution to QuickLRU

8e64e7b

garrettjstevens merged commit 37e3512 into master Oct 30, 2019

cmdcolin deleted the cra_rescripts branch November 21, 2019 02:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Electron build of JBrowse desktop #520

Electron build of JBrowse desktop #520

garrettjstevens commented Sep 24, 2019 •

edited

Loading

garrettjstevens commented Sep 29, 2019

rbuels commented Sep 30, 2019 via email

garrettjstevens commented Oct 1, 2019

codecov bot commented Oct 6, 2019 •

edited

Loading

garrettjstevens commented Oct 13, 2019

garrettjstevens commented Oct 30, 2019

cmdcolin commented Oct 30, 2019

cmdcolin Oct 30, 2019

garrettjstevens Oct 30, 2019

cmdcolin Oct 30, 2019

cmdcolin Oct 30, 2019

cmdcolin Oct 30, 2019

cmdcolin Oct 30, 2019

Electron build of JBrowse desktop #520

Electron build of JBrowse desktop #520

Conversation

garrettjstevens commented Sep 24, 2019 • edited Loading

garrettjstevens commented Sep 29, 2019

rbuels commented Sep 30, 2019 via email

garrettjstevens commented Oct 1, 2019

codecov bot commented Oct 6, 2019 • edited Loading

Codecov Report

garrettjstevens commented Oct 13, 2019

garrettjstevens commented Oct 30, 2019

cmdcolin commented Oct 30, 2019

cmdcolin Oct 30, 2019

Choose a reason for hiding this comment

garrettjstevens Oct 30, 2019

Choose a reason for hiding this comment

cmdcolin Oct 30, 2019

Choose a reason for hiding this comment

cmdcolin Oct 30, 2019

Choose a reason for hiding this comment

cmdcolin Oct 30, 2019

Choose a reason for hiding this comment

cmdcolin Oct 30, 2019

Choose a reason for hiding this comment

garrettjstevens commented Sep 24, 2019 •

edited

Loading

codecov bot commented Oct 6, 2019 •

edited

Loading