Transition from contextWindow to postMessage by twolfson · Pull Request #1984 · karma-runner/karma

twolfson · 2016-03-11T07:08:36Z

I am currently attempting to get Karma running with Electron. There are a lot of benefits to this:

Can hide Electron windows like PhantomJS
Can load modules easily via require
Environment is identical to production if we are building an Electron app (which I am)
Built-in screenshot support for visual diffs

Unfortunately, the current Karma implementation is incompatible with Electron.

If we use an iframe, we lose the Node.js integration and must polyfill it via window.parent. This works great for top level modules but submodules have their global context executed in the window.parent which makes using mocha impossible =/

For reference, here was our polyfill to date: https://github.com/twolfson/karma-electron/blob/3.0.1/lib/node-integration-iframe.mustache.js

Other alternatives from an iframe for Electron are window.open, <webview>, and launching multiple BrowserWindow instances. Unfortunately, these options are executed in separate processes so we cannot use window.opener as per usual.

window.open defines a proxy where we can run eval and postMessage
- Documentation: https://github.com/atom/electron/blob/v0.36.10/docs/api/window-open.md
<webview> defines executeJavaScript and send
- Both of these methods are similar to window.open except they can receive responses whereas neither eval nor postMessage can directly receive responses (although we can send a corresponding postMessage reply or build a bridge library to match requests/responses)
- Documentation: https://github.com/atom/electron/blob/v0.36.10/docs/api/web-view-tag.md
Launching new BrowserWindow via Electron seems too custom to Electron to be reusable elsewhere
- I should note that we would have sendSync in the browser which allows for synchronous function calls and responses but this only works when sending from renderer to main processes

I decided to use window.open since it doesn't require any duplicate logic for other browsers and is easily polyfilled for older browsers to use contextWindow.

In this PR, I have a proof of concept working with the karma-electron test suite but the PR is far from done.

Matching test suite: https://github.com/twolfson/karma-electron/tree/ba7d54bf4d941200c9c15d94ebf0cbfd7546d5c7

What are your thoughts on this PR?

googlebot · 2016-03-11T07:08:38Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

twolfson · 2016-03-11T07:12:37Z

I signed it!

googlebot · 2016-03-11T07:12:39Z

CLAs look good, thanks!

dignifiedquire · 2016-03-11T11:59:04Z

static/context.html

-    } else {
-      window.parent.karma.setupContext(window);
-    }
+    (function () {


All this code should still be in a separate file rather than inlined here.

Yea, definitely. This was mostly to get the proof of concept up and out the door =)

dignifiedquire · 2016-03-11T12:08:57Z

Hey,

first thanks for all this work! In general I like the idea of using window.open, but we need to be a 100% certain we can polyfill and still support all browsers back to IE7 as we do currently. I can't ship a version that drops that compatibility as too many people are relying on this.

twolfson · 2016-03-11T18:41:12Z

The polyfill would be less of an actual polyfill and more of a fallback implementation for Electron:

// Define a remote call method for Karma
var parentWindow = window.opener || window.parent;
var callParentKarma = function (method, args) {
  parentWindow.karma[method].apply(parentWindow.karma, args);
};

// If we don't have access to the window, then use `postMessage`
// DEV: In Electron, we don't have access to the parent window due to it being in a separate process
// DEV: We avoid using this in Internet Explorer as they only support strings
//   http://caniuse.com/#search=postmessage
if (!parentWindow.window) {
  callParentKarma = function (method, args) {
    parentWindow.postMessage({method: method, arguments: args}, window.location.origin);
  };
}

I think the next iteration of this PR won't be a fully polished product but another proof of concept demonstrating the concept works in other browsers

twolfson · 2016-03-14T05:43:09Z

Alright, I was able to make a clean transition for everything (or at least I think it's clean). There are still some rough edges in this PR (e.g. lingering TODOs, using PhantomJS over Chrome/FF so I can dev faster) but the majority of it is here.

I know that a large PR can be frustrating as a reviewer. As a result, I had some interim branches for you to glance over:

twolfson/karma@87c56e3...twolfson:dev/postmessage.work

Also, here was my initial game plan although I derailed from the path at some point:

https://gist.github.com/twolfson/f7d257966d7cdafdf30c

twolfson · 2016-03-20T21:42:08Z

@dignifiedquire Would it be possible to get a review on this PR? I'm more than happy to break it down into smaller pieces if that helps

dignifiedquire · 2016-03-20T21:45:28Z

@twolfson yes I will try to get to it tomorrow

dignifiedquire · 2016-03-23T19:44:48Z

.travis.yml

 sudo: false
 language: node_js
 node_js:
-  - "0.10"


We need to pass our tests on all these platforms, please leave them in here

These were removed for local development. I will restore them

dignifiedquire · 2016-03-23T19:54:13Z

Alright this is starting to take nice form, I left some smaller comments inline.
The main thing I'm pondering right now is if I want to get this in before or after shipping 1.0 (which I wanted to do last week already cough).
I'm going to test this on some of my projects tomorrow to see if there are any compat issues that I can see.

Could you also do a rebase please of latest master so it merges cleanly?

twolfson · 2016-03-23T20:07:56Z

Hooray, thanks for the review =) Most of the comments were on files/edits that were done for local development (i.e. make everything headless/simple to make iterations as fast as possible).

They were left in due to this PR still being up in the air. Now that it sounds like this will eventually be landed, then I agree that it's best to take them out and add finishing polish.

With respect to before/after 1.0, it's prob best to hold this off until after 1.0. I suggest shipping this feature as standalone as possible since it could break builds (unlikely but plausible) and we want to be easily revertable.

With respect to the requested changes in this PR, I will take care of them by the end of the weekend.

twolfson · 2016-03-23T20:10:51Z

.travis.yml

+    recipients:
+      - todd@twolfson.com
+    on_success: change
+    on_failure: change


Note to self: Remove these edits as well

twolfson · 2016-03-23T20:18:57Z

Not sure if you caught this but what were your thoughts on the folder structure (i.e. relocating some client/ files to a common/ and creating a context/ folder)?

I know that some developers like to keep the top level folder as clean as possible so an alternative might be a browser/ folder which contains each of those folder or keep all the files in a browser/ and rename client/karma.js to browser/parent-karma.js

twolfson · 2016-03-24T17:24:13Z

I'm realizing that when we introduce customContextFile and customDebugFile that we will have a major breaking change when this PR's new context/debug file formats land. I'm going to break those off into another PR so we can sneak it in before 1.0.

dignifiedquire · 2016-03-25T15:03:10Z

Nice! I will be travelling the next week, so I might be slow to respond, but going to test this out and see how it works :)

…ularity BREAKING CHANGE: Our `context.html` and `debug.html` structures have changed to lean on `context.js` and `debug.js`. This is in preparation for deeper `context.js` changes in karma-runner#1984. As a result, all `customContextFile` and `customDebugFile` options much update their format to match this new format.

twolfson · 2016-03-25T19:23:05Z

This PR is now on top of the latest changes in #2019 and #2020

twolfson · 2016-04-14T22:03:29Z

Bump on this PR and its related PRs =/

…ularity BREAKING CHANGE: Our `context.html` and `debug.html` structures have changed to lean on `context.js` and `debug.js`. This is in preparation for deeper `context.js` changes in karma-runner#1984. As a result, all `customContextFile` and `customDebugFile` options much update their format to match this new format.

twolfson · 2016-04-15T03:31:04Z

This PR should be good to review now as well 👍

twolfson · 2016-04-27T01:41:16Z

Bump on this issue again =/

twolfson · 2016-05-26T18:13:22Z

Bump on this issue again =/

dignifiedquire · 2016-06-26T15:35:30Z

@twolfson I finally got around to releasing 1.0. Sorry for the massive delay on this but I had a lot to do on my day job. Reviewing this now, in the meantime could you please rebase this onto the latest master?

… Electron support via postMessage

twolfson · 2016-06-26T16:21:12Z

I have rebased the commit to the latest master

dignifiedquire · 2016-06-26T17:54:52Z

Thanks a lot @twolfson, going to merge this and put it into the new release

dignifiedquire · 2016-06-26T18:15:59Z

@twolfson released as 1.1.0

twolfson · 2016-06-26T20:16:04Z

woot, awesome. Thanks =D

dignifiedquire reviewed Mar 11, 2016
View reviewed changes

dignifiedquire added WIP discuss labels Mar 11, 2016

twolfson mentioned this pull request Mar 12, 2016

Added commit message validation to Travis CI #1989

Merged

twolfson force-pushed the dev/use.context.window.sqwished branch from 76e8d9f to a073481 Compare March 14, 2016 05:39

twolfson force-pushed the dev/use.context.window.sqwished branch from a073481 to b1e23fa Compare March 14, 2016 05:48

twolfson mentioned this pull request Mar 18, 2016

No customContextFile causes Windows to fail #2000

Closed

dignifiedquire self-assigned this Mar 21, 2016

dignifiedquire reviewed Mar 23, 2016
View reviewed changes

dignifiedquire removed the discuss label Mar 23, 2016

twolfson reviewed Mar 23, 2016
View reviewed changes

twolfson force-pushed the dev/use.context.window.sqwished branch from 7efa81d to 7dd2aeb Compare March 25, 2016 19:22

twolfson mentioned this pull request Apr 6, 2016

electron-prebuilt upgraded to latest release twolfson/karma-electron#2

Merged

twolfson force-pushed the dev/use.context.window.sqwished branch 2 times, most recently from 489a921 to 19d81d0 Compare April 15, 2016 03:30

twolfson mentioned this pull request Apr 25, 2016

[Request] add source map support twolfson/karma-electron#3

Closed

dignifiedquire added needs: review and removed WIP labels Apr 27, 2016

dignifiedquire mentioned this pull request Jun 26, 2016

Updated version of postMessage transition #2195

Closed

refactor(client): Transferred lots of client logic to context, adding…

81b1f04

… Electron support via postMessage

twolfson force-pushed the dev/use.context.window.sqwished branch from 19d81d0 to 81b1f04 Compare June 26, 2016 16:20

dignifiedquire removed the needs: review label Jun 26, 2016

dignifiedquire merged commit fc0421f into karma-runner:master Jun 26, 2016

twolfson deleted the dev/use.context.window.sqwished branch June 26, 2016 20:17

pirelenito mentioned this pull request Sep 14, 2016

Specify Karma version compatibility twolfson/karma-electron#13

Closed

Omrisnyk mentioned this pull request Oct 27, 2023

[Snyk] Fix for 2 vulnerabilities Omrisnyk/npm-lockfiles#122

Open

Omrisnyk mentioned this pull request Dec 15, 2023

[Snyk] Security upgrade karma from 2.0.3 to 3.0.0 Omrisnyk/npm-lockfiles#132

Open

Conversation

twolfson commented Mar 11, 2016

Uh oh!

googlebot commented Mar 11, 2016

Uh oh!

twolfson commented Mar 11, 2016

Uh oh!

googlebot commented Mar 11, 2016

Uh oh!

dignifiedquire Mar 11, 2016

Choose a reason for hiding this comment

Uh oh!

twolfson Mar 11, 2016

Choose a reason for hiding this comment

Uh oh!

dignifiedquire commented Mar 11, 2016

Uh oh!

twolfson commented Mar 11, 2016

Uh oh!

twolfson commented Mar 14, 2016

Uh oh!

twolfson commented Mar 20, 2016

Uh oh!

dignifiedquire commented Mar 20, 2016

Uh oh!

dignifiedquire Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

twolfson Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

dignifiedquire commented Mar 23, 2016

Uh oh!

twolfson commented Mar 23, 2016

Uh oh!

twolfson Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

twolfson commented Mar 23, 2016

Uh oh!

twolfson commented Mar 24, 2016

Uh oh!

dignifiedquire commented Mar 25, 2016

Uh oh!

twolfson commented Mar 25, 2016

Uh oh!

twolfson commented Apr 14, 2016

Uh oh!

twolfson commented Apr 15, 2016

Uh oh!

twolfson commented Apr 27, 2016

Uh oh!

twolfson commented May 26, 2016

Uh oh!

dignifiedquire commented Jun 26, 2016

Uh oh!

twolfson commented Jun 26, 2016

Uh oh!

dignifiedquire commented Jun 26, 2016

Uh oh!

dignifiedquire commented Jun 26, 2016

Uh oh!

twolfson commented Jun 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants