Open orther opened 8 years ago
Why a shim is needed for Electron:
Electron provides built-in modules for developing native desktop applications and those modules are accessed by requiring the electron
module like so:
// JavaScript example of accessing Electron modules
const electron = require('electron');
const app = electron.app;
const screen = electron.screen;
;; ClojureScript example of accessing Electron modules
(def electron (js/require "electron"))
(def app (.-app (.-remote electron)))
(def screen (.-screen electron))
When running tests with karma in Electron the require
function is not available and you will get this error: ReferenceError: require is not defined
.
NOTE: You will only get that error if you are using require
in the tests or code you're testing. If you are not using require
you do NOT need a shim.
After some googling I came across this karma.shim.js which solves the missing require
error with this line:
window.require = window.top.require;
I'd like to mention that if it makes sense, I can just clean up my current code and make a pull request with working code to support running tests in Electron using karma with the shim.
Hi @orther
This is great work. Thank you for doing this and then taking the time to explain it so well!
Re Outline 2.i It is probably necessary to create the file for the shim dynamically even though the contents are static. When doo executes the tests, the file is in a jar, which can be accessed by a Java process but not by Node. It needs to be copied into the file system so that Node can find it.
Answers:
runner-path!
copies an io/resource
into tmp
and returns the absolute path. Instead of copying the function into doo.karma
, move it to doo.utils
, and use it from both doo.karma
and doo.core
.doo
, but it is also possible to prepend "shims" by using the :foreing-libs
option of the compiler. We should add the shims that are absolutely necessary to the runner but not to specific projects. #25 shows why it is not necessary to expose the functionality to users. Your solution is good as it is.karma.conf.js
file. To use multiple conf
files, you would need multiple karma instances. If you are going to launch electron, then the electron conf
file is needed. If that conf
file prevents other launchers from working, we would need to launch other karma servers. As far as I can tell, this functionality is not needed.Also, the Nashorn PR #66 and the Canary PR #73 can be useful reference.
Feel free to submit your changes as a PR and we can keep discussing it there.
Thanks again!
Thanks @bensu you're response was exactly what I was looking for.
I am pretty close to having the code in shape for a pull request but I am having a problem with the boot tests not passing. Right now boot loads all the files in src
and test
directories and runs the test with phantomjs. That will not work for the electron tests/src so I need to exclude specific files. Do you know the best way to do that with boot/ boot-cljs-test? I'm sure I'll be able to figure it out with some banging my head against the wall but I figured I'd ask.
To rephrase: boot is adding src/example/electron.cljs
, test/example/electron_runner.cljs
, and test/example/electron_test.cljs
to the compilation which breaks phantom. ClojureScript normally adds only what is necessary to the build, which allows you to solve the problem through :main
. Is that the problem? Then I don't know how to solve it but @crisptrutski might have an idea.
@orther @bensu Just pushed 0.2.1
of boot-cljs-test
which fixes usage with karma (and electron)
@orther Any progress? I recently attempted adding karma-slimerjs
and karma-phantomjs
in this branch which might be useful for this enhancement.
@orther the latest master has now a beta implementation of electron
. Would you mind trying it?
It implements the bare minimum from your fork, and if you are up for it we could continue to add work from your fork.
@bensu I switched my focus to mostly JS (paid projects) over the last couple months but I've been meaning to get back to CLJS hobby project(s). I welcome this as a catalyst!
I have pulled the master branch and can successfully run the core tests in Electron. I am working on getting the Electron tests from my fork to run/pass. Once I get the Electron tests working I'll let you know and/or make a pull request.
I have created a new branch on my fork based on upstream master and added the changes to support electron testing here: https://github.com/orther/doo/tree/electron-test
You can see I added a shim file and build-ids to use it. https://github.com/bensu/doo/compare/master...orther:electron-test
Great!
I like the tests but I see that adding the shim implies many other changes. Why is the shim necessary and is it common to all ClojureScript Electron projects?
I try not to add shims if we can avoid it.
You can see why the shim is required in my first comment of this issue here: https://github.com/bensu/doo/issues/90#issuecomment-168115555
It would be great if instead of putting the shim in the library, the require error could be fixed in the (example) project but I haven't found a way to do that. I haven't looked into this for some time so I can't fully remember the details and just copied over what my working solution was from a few months ago. I'll look into this further.
Hi @orther,
You are right, you were perfectly clear about the shim. Sorry for the oversight. After a third look, the shim looks good. It is probably related to how Karma evaluates the test code inside the JavaScript environment, and the need to reach the top window frame where the special Electron objects where added.
The code is ready for a PR. I'll ask you to change is that if you are moving runner-path!
, there should be now duplicate left.
I'll try to clean this up and create PR tomorrow afternoon.
I also wanted to mention that I am going to look into http://electron.atom.io/spectron/ and if it makes sense to support it with doo.
I have added support for Electron running tests with the karma-electron-launcher in my fork. Before I make a proper pull request, I'd like advice on handling the creation and loading of a shim file.
Please ignore my current crude implementation as I plan to clean all this code up once I decide the best way forward. Below is the basic outline of how I added support for the Electron shim:
doo.core/runner-path!
function to the doo.karma namespace.karma.config.js
file does. The reason I did it this way is because I was just copying how the phantomjs shim file was created.->karma-opts
function, if Electron is one of the launchers the temp shim file is created usingrunner-path!
and it's path is added as the first item in the"files"
list (see karma.clj#L88-L89)."files"
list is important since it determines the order files are loading in thekarma.config.js
that is create. For a shim, it's likely that it needs first or at least before tests are loaded.My Questions:
runner-path!
function I copied to thedoo.karma
namespace to create the tempelectron-shims.js
.)lein-doo
to add a file to the top of the"files"
array in thekarma.config.js
file? This would allow projects to add custom shims to karma if wanted.karma.config.js
for each unique config based on"files"
array? (This could be an advanced feature that is decided on at a later date. I currently don't have a use case for running Electron specific tests in other launchers.)I know comments in issues #34 and #43 mention the need to come up with a general solution to a more composable karma config. I believe shim support could be solved with that but, I don't know the status of that solution and believe that since this shim is specific to a single launcher it is more acceptable to solve it in a hard coded one off manner if necessary.
NOTE: I'll explain why a shim is needed as comment on this issues shortly.