Switch from electron-protocol-serve to file: URLs #476

bendemboski · 2020-05-18T19:43:59Z

electron-protocol-serve seems to have taken us too far off the beaten path, so this changes ember-electron to set up projects to load from file: URLs instead of the custom protocol. This requires a couple of workarounds to make Ember apps work properly when loaded from file: URLs -- all explained in detail in the new FAQ page. This gets us sourcemaps back in Electron 7+, and also removes the need for a variety of minor workarounds for security/sandboxing issues caused by our use of a custom protocol.

This PR also includes some related changes that simplify the infrastructure supporting ember electron:test, and improve our testing story a bit (i.e. the ember-electron's own tests).

Fixes #442

bendemboski · 2020-05-18T19:46:29Z

@rwwagner90 and @jacobq if you are able to try out these changes in your apps I'd very much appreciate it. I've tried them in mine, and they are working, but I'd like some more verification. Also, I'd love a verification that the update instructions from the previous beta work.

RobbieTheWagner · 2020-05-18T21:33:59Z

@bendemboski thanks for working on this! I will try it out in the next day or two.

RobbieTheWagner

This mostly LGTM!

forge/files/src/index.js

RobbieTheWagner · 2020-05-18T23:27:16Z

tests/dummy/app/templates/docs/faq/routing-and-asset-loading.md

+
+When running an Electron app, we serve the files out of the filesystem using a `file:` URL, which doesn't give us the flexibility of a web server -- the URL path cannot be a logical location, but must be a filesystem path that points directly to `index.html`. For example, when running a development build, the path might be `file:///Users/me/projects/my-app/electron-app/ember-dist/index.html`. We can't use the URL path to specify the Ember route because telling the browser to load `file:///Users/me/projects/my-app/electron-app/ember-dist/index.html/my-route` will not work -- it will look for a file named `my-route` inside a directory named `index.html`, and won't find it.
+
+So we have to go back to an older method of specifying logical locations, which is to use the URL hash. Fortunately, Ember supports this via the [HashLocation](https://api.emberjs.com/ember/release/classes/HashLocation) since that was the only option in older browsers. Since URL hashes don't affect how `file:` URLs are resolved to paths, we can tell the browser to load `file:///Users/me/projects/my-app/electron-app/ember-dist/index.html#my-route`, and it will load the `index.html` and then Ember can resolve the route using the path contained in the URL hash.


Suggested change

So we have to go back to an older method of specifying logical locations, which is to use the URL hash. Fortunately, Ember supports this via the [HashLocation](https://api.emberjs.com/ember/release/classes/HashLocation) since that was the only option in older browsers. Since URL hashes don't affect how `file:` URLs are resolved to paths, we can tell the browser to load `file:///Users/me/projects/my-app/electron-app/ember-dist/index.html#my-route`, and it will load the `index.html` and then Ember can resolve the route using the path contained in the URL hash.

So we have to go back to an older method of specifying logical locations, which is to use the URL hash. Fortunately, Ember supports this via the [HashLocation](https://api.emberjs.com/ember/release/classes/HashLocation) since that was the only option in older browsers. Since URL hashes don't affect how `file:` URLs are resolved to paths, we can tell the browser to load `file:///Users/me/projects/my-app/electron-app/ember-dist/index.html#/my-route`, and it will load the `index.html` and then Ember can resolve the route using the path contained in the URL hash.

Hash routes need to be index.html#/route-name

jacobq · 2020-05-19T20:50:11Z

blueprints/ember-electron/index.js

+      ].join(' ')));
+    }
+
+    const locationTypeRegex = /(\slocationType:)/;


This is a bit pedantic, but whitespace is allowed between the key name and the : so it might be worth adding \s* to these RegExps to cover that case also.

[" foo:123", " foo : 123" ].map(s => /(\sfoo:)/.test(s)) // --> [ true, false ] [" foo:123", " foo : 123" ].map(s => /(\sfoo\s*:)/.test(s)) // --> [ true, true ]

Yeah, I suppose. I was mainly trying to target the output of the default app blueprints. The leading \s was to make sure that the locationType wasn't a substring, like if the user had another setting that was colocationType or something...probably even less likely than the user putting spaces before the : 😄 I don't want to go crazy trying to handle all kinds of cases of modifications to the app-blueprint-generated, but I think that extra \s* is super reasonable -- I'll add it.

Got to draw the line somewhere, but for me that point is more like supporting { [eval("'f' + 'oo'")]: ...} 😱

jacobq · 2020-05-19T20:54:17Z

forge/files/src/handle-file-urls.js

+const fs = require('fs');
+const { promisify } = require('util');
+
+const access = promisify(fs.access);


Remind me again why we can't use fsPromises... Need to support Node < 10.x?

I see this in package.json:

"engines": { "node": "10.* || >= 12" },

Uh, yeah, we kinda recently dropped node 8 support, so that's why we were avoiding it before. However, this is a different case, because this code runs in the Electron main process, not the user's Node environment, so the engines in package.json isn't relevant here, just the version of Electron that the user is running.

But Electron has been on Node 10+ since 3.x, and I'm quite confident that there are other reasons this code would break on Electron <=2, not the least of which is the usage of fileURLToPath in this same file, which wasn't introduced until Node 10.12/Electron 5 😆

I don't think we have an official policy on which versions of Electron we do and don't target, but since:

This is all blueprint-generated code that the user could modify/replace if they need to

We're in the midst of a major version bump

I think we're fine keeping the usage of fileURLToPath and using fs.promises. I'll make the change.

I think we should maybe support Electron 5+ or whatever we had to update Electron Forge for?

FWIW the Electron team only supports the latest 3 stable branches. So now that 9.0.0 is out I think that means versions older than 7.x are no longer supported. I am fine with 5+, but my gut would be to target actively supported versions.

grumble grumble https://travis-ci.com/github/adopted-ember-addons/ember-electron/jobs/337264506#L610

I would be fine with Electron 7+, and I would honestly also be fine with bumping node minimum to 12, but ember-cli supports 10 until April 2021.

Yeah, this is kinda silly because eslint is warning us about code that runs in Electron, not the environment that the engines field is describing. So I don't think bumping our minimum required node version makes any sense because I don't think we use any fs.promises in code that actually runs in the user's node environment, so we'd be dropping support just to make the linter happy.

That being said, I do think it makes sense to get more specific about what Electron versions we support, but haven't put any thought into really how to accomplish that -- I guess it would just be documentation and then testing against all supported (major) versions?

Yeah, I think we would mainly list in the documentation that we don't support earlier Electron versions, but that it may still work.

jacobq · 2020-05-19T21:09:16Z

forge/files/src/index.js

+    try {
+      require('devtron').install();
+    } catch (err) {
+      console.log('Failed to install Devtrom: ', err);


Typo? Devtrom -> Devtron

P.S. Thank you for making this message more meaningful. I hadn't personally ran into it, but overly generic error messages drive me nuts in logs.

Haha, yeah, I noticed that myself and have it fixed locally (new commit coming soon)

jacobq · 2020-05-19T21:11:20Z

forge/files/tests/index.js

+  try {
+    require('devtron').install();
+  } catch (err) {
+    console.log('Failed to install Devtrom: ', err);


Same typo as previously: Devtrom -> Devtron

jacobq · 2020-05-19T21:12:53Z

forge/files/tests/index.js

+  try {
+    await installExtension(EMBER_INSPECTOR);
+  } catch (err) {
+    console.log('Failed to install Ember Inspector: ', err)


Same missing ; as before...

lib/test-runner.js

node-tests/fixtures/ember-test/test-index-extra.js

jacobq · 2020-05-19T21:50:46Z

node-tests/unit/blueprint-test.js

+
+      await blueprint.updateEnvironmentConfig();
+
+      let factory = require(path.join(process.cwd(), 'config', 'environment.js'));


Another nit: can we reduce the <cwd>/config/environment.js duplication? e.g.

mkdirSync('config'); const newEnvJsPath = path.join(process.cwd(), 'config', 'environment.js'); copyFileSync(environmentJs, newEnvJsPath); await blueprint.updateEnvironmentConfig(); let factory = require(newEnvJsPath);

tests/dummy/app/templates/docs/faq/common-issues.md

tests/dummy/app/templates/docs/faq/routing-and-asset-loading.md

jacobq · 2020-05-19T22:08:27Z

tests/dummy/app/templates/docs/guides/upgrading.md

-  protocol,
-});
-```
+The equivalent of `ember-electron/main.js` is `electron-app/src/index.js`, so you'll want to put the contents of your `main.js` there (note that `src/index.js` is specified by the `main` entry in `electron-app/package.json`, so you can name it whatever/put it wherever you want as long as you update the `main` entry accordingy). Because the Electron project structure differs a little from `ember-electron` 2.x's, you'll need to replace anywhere you reference the path to the Ember application directory (`../ember`) with the new path (`../ember-dist`).


Typo: accordingy -> accordingly

tests/dummy/app/templates/docs/guides/upgrading.md

jacobq

❤️ So glad at the thought of having source maps fixed -- also seems like this removes more code than it adds, which is always good in my book.

I read through the changes but have not yet tried using it in my app (still have some things that rely on the URL starting with serve:// etc). Will plan to make time to try it in the next few days, but we'll see :/

jacobq · 2020-05-19T23:37:12Z

:D Change engines to ^10.17.0 || 12.* || >= 14?

…

On Tue, May 19, 2020, 6:27 PM Ben Demboski ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In forge/files/src/handle-file-urls.js <#476 (comment)> : > @@ -0,0 +1,29 @@ +const { protocol } = require('electron'); +const { fileURLToPath } = require('url'); +const path = require('path'); +const fs = require('fs'); +const { promisify } = require('util'); + +const access = promisify(fs.access); *grumble grumble* https://travis-ci.com/github/adopted-ember-addons/ember-electron/jobs/337264506#L610 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#476 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMOZM2F76CUGM7VO4W6BG3RSMIVJANCNFSM4NELYXPA> .

jacobq · 2020-05-20T03:02:24Z

Curious about the electron 9 breakage with Devtron. Looks like we're seeing TypeError: electron.BrowserWindow.addDevToolsExtension is not a function (as if it were called statically instead of on an instance?). When I tried to build my app (using ember-electron@3.0.0-beta.2) with electron 9, I saw a test failure in CI like the following, does this sound related?

Loading extension at /my-app/electron-app/node_modules/devtron failed with: The 'manifest_version' key must be present and set to 2 (without quotes). See developer.chrome.com/extensions/manifestVersion.html for details.

I probably shouldn't be worried, but the latest release of DevTron is 1.4.0 and was made 4 years ago, which makes me think it could easily stop working with a major electron version bump...

bendemboski · 2020-05-20T05:47:38Z

Haha, they made my life super difficult by releasing Electron 9 in the middle of this PR 😆

There are two issues at play, this one which affects both Ember Inspector and devtron, and is failing the tests (but has an easy workaround) and this one which I just filed, which only affects devtron, and which I have no idea if there's a workaround...seems like not.

Looking in devtron, its manifest doesn't look at all like I think a manifest for an extension should...or it's a super duper duper old format that maybe Chromium and/or Electron dropped support for? Anyway, nothing to do with how we install it -- it's just broken on Electron 9.

bendemboski · 2020-05-20T18:03:22Z

@rwwagner90 what can we do about the codeclimate failure? I don't agree that addressing any of the issues would actually make the code clearer, although happy to discuss -- how do we get the checks passing?

RobbieTheWagner · 2020-05-21T16:04:38Z

@bendemboski you should be able to log in and approve them. I logged in and approved them, so should be good to go!

bendemboski · 2020-05-21T16:14:59Z

@rwwagner90 thanks!

In ember-electron 2.x, the `electron:test` command would build the Ember app into a temp location somewhere, so we'd have to jump through a bunch of hoops to pass that location to the Electron app so it could load the Ember app into the browser window. In 3.x, we changed it so that the Ember app is always built into `electron-app/ember-test`, so we don't need to pass this extra information around at runtime. So we're now simplifying and better organizing the test infrastructure: * Remove the code that handles passing the Ember app location through to the test main process * Put the `electron-protocol-serve` setup code into a blueprint-generated utility function. This way the production and test versions of the app use the same code, and it's easier to see the correspondence between the two different usages * Reorganize the test support code a bit for better flexibility -- rather than the test `index.js` just requiring a single file that runs all the test main process code, it imports some functions and calls them. This makes it a bit easier to read and understand, makes it easier to share code between the production and test main processes, and also allows more flexibility for users to customize, since the functionality that `ember-electron` supplies isn't all-or-nothing * await the Ember Inspector installation before opening the browser window, to make sure we don't have a race condition. This should fix the intermittent failures we've seen in the test that verifies that we install Ember Inspector. * Make end-to-end test more robust -- in this test we paste some code into the test `index.js` to verify some functionality, so rather than putting it at module scope, we put it in a function that we call so we don't have to worry about naming conflicts/duplicate imports between the blueprint-generated code and the test code we paste into the same file

When running not in CI, the electron command unit test was showing a building spinner, which pollutes the test output and can cause the process to not exit because of a leaked interval timer, so stub out the progress methods

`electron-protocol-serve` seems to have taken us too far off the beaten path, so this changes `ember-electron` to set up projects to load from file: URLs instead of the custom protocol. This requires a couple of workarounds to make Ember apps work properly when loaded from file: URLs -- all explained in detail in the new FAQ page. This gets us sourcemaps back in Electron 7+, and also removes the need for a variety of minor workarounds for security/sandboxing issues caused by our use of a custom protocol. Fixes #442

Unfortunately, due to bugs in Electron 9, we can't use devtron (electron/electron#23676). We could at least get Ember Inspector to load (using the workaround for electron/electron#23656), but hopefully Electron will turn around a quick fix at least for that second issue so we don't have to add a temporary workaround to the blueprint-generated code...

bendemboski requested review from RobbieTheWagner and jacobq May 18, 2020 19:44

bendemboski force-pushed the file-urls branch from cdd76e6 to 67cb910 Compare May 18, 2020 22:56

RobbieTheWagner reviewed May 18, 2020

View reviewed changes

forge/files/src/index.js Outdated Show resolved Hide resolved

forge/files/src/index.js Show resolved Hide resolved

forge/files/src/index.js Show resolved Hide resolved

RobbieTheWagner reviewed May 18, 2020

View reviewed changes