twolfson / karma-electron

Karma launcher and preprocessor for Electron
The Unlicense
59 stars 21 forks source link

require in renderer process #11

Closed friedrith closed 8 years ago

friedrith commented 8 years ago

Hi,

This module is a good idea because renderer process is very specific. However I have a very simple but I didn't find any solution:

Is it normal and is there a option to change this base path (basePath in karma doesn't change this path) ?

I hope my question is quite understandable.

Thanks

twolfson commented 8 years ago

Could you elaborate more on "require in renderer process use index.html path as base whereas your plugin use the .js file as base."? Maybe provide some example code so I can understand better

danforbes commented 8 years ago

I think I might understand this question. I have an Angular controller defined in <application root>/app/home/home.ctrl.js. In this controller, I say:

var foo = require(./app/foo);

And this works great when I run my application by simply spawning electron in the directory containing my main.js files. However, when I run my Karma tests, I get an error that tells me;

Cannot find module '<application root>/app/home/./app/log'

Is this because your preprocessor is setting the __dirname variable?

I have a temporary workaround. Rather than using require to import my module, I source it in my index.html file. In the file I'm sourcing, I just set process.<some property> to some value (e.g. a function, an object) that will allow me to access the method(s) defined in my module.

twolfson commented 8 years ago

The current require is polyfilled to match the experience someone should expect from electron/Node.js. The require resolves relative to the current script file (subsequent requires to a script loaded from index.html are relative to the file itself, not the initial require context). To reiterate:

views/index.html:

<!-- Loads `views/../js/foo.js` (i.e. `js/foo.js`) -->
<script>require('../js/foo');</script>

js/foo.js:

// Loads `./bar.js` (i.e. `js/bar.js`)
require('./bar');
// This is the context we load scripts as
//    That is: The file you are loading in `karma` is a JS file
//    someone wouldn't/shouldn't expect it to load from the HTML context
//    as the file isn't HTML

The reason we need to polyfill __dirname and __filename is karma executes tests from a localhost server. Since this is an HTTP context and not file://, electron doesn't know how to orient itself so we must redefine those would-be values

danforbes commented 8 years ago

Well, that's all fair enough, I suppose, but I think the reason that this issue was raised is because there seems to be an incongruity between the behavior in the normal runtime environment versus the testing environment. That is, paths that resolve in the normal runtime environment do not resolve in the testing environment and vice versa.

twolfson commented 8 years ago

Ah, I think I understand now. I will rephrase the problem to verify we are on the same page:

The question seems to be something like: Can we add basePath to the require resolution path?

My answer would be: We could but I would prefer to avoid doing so as it would lead to unpredictable development/testing behavior. As a developer, I would expect a require for ./foo/bar.js to resolve relative to the same file in both a normal electronand testing environment. If we were to add basePath to the require resolution path, then we could either load separate files or get a miss in development but hit in testing.

danforbes commented 8 years ago

Thanks for the response - glad I was able to help clarify!

I don't have the same depth of knowledge that you have, but here're my two cents: the testing environment should behave exactly like the normal runtime environment. I totally hear what you're saying, vis-a-vis reasonable developer expectations, but the aim of a testing environment should be to mimic the normal runtime environment, regardless of whether or not the normal runtime environment conforms to "reasonable" expectations.

You seem to indicate that "fixing" this problem could result in incongruous behavior between the normal runtime environment and the testing environment, but as far as I'm concerned the problem we're discussing right now is that there is an incongruity of behavior between the normal runtime and testing environments.

twolfson commented 8 years ago

Nope, the behaviors should currently be the same as karma-electron was designed under that philosophy (i.e. karma-electron matches electron as closely as possible). My current interpretation of the issue is to add another path to require which is karma's basePath option

That being said, I could be wrong about missing some inconsistency between electron and karma-electron's polyfill. If you have any examples, please let us know =)

danforbes commented 8 years ago

I've prepared some code to demonstrate the problem I'm trying to describe. DanForbes/electron at test/karma-electron/require

/app/home/home.ctrl.js requires /app/node-scripts/my-script.js, which resolves properly in the Electron runtime environment (the "Whoa, this works great message!" comes from the required file). When I try to run my tests, though, I get the following error message:

Uncaught Error: Cannot find module '<project root>/app/home/./app/node-scripts/my-script' at module.js:457

Let me know if I'm missing something, doing something wrong or if you have any questions.

twolfson commented 8 years ago

Ah, I see. Yea, there's a discrepancy between how we would expect to load the JS files. You are loading via:

<script src="./app/app.js"></script>

whereas I would expect to use

<script>require('./app/app.js');</script>

or have a browser.js which then requires an application constructor and initializes it (so the tests can have the same behavior and we avoid globals altogether).

It's been a while since I've touched this repo but I recalled setting up a similar gist to verify we had the majority of the behavior matched. However, now that I look at it again we never tested these different loading mechanisms:

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

I want to take some time to test further, reflect on which behavior should be considered the default/why, and likely add an option for the opposite setup (e.g. requireRelativeToPage). Here are my current thoughts:

danforbes commented 8 years ago

Glad I was able to clearly illustrate the issue. Also, thanks for describing the way in which you would have structured things in order to avoid this issue. Although I'm a software engineer by profession, I'm not employed as a web dev and am keenly aware of the fact that I don't write web development code like a professional...so I'm happy to hear how a more seasoned web developer would structure things!

As the repository linked in my last comment indicates, I followed a tutorial to build the "bones" of my minimal Electron app. As you can see in the fourth code block in the Controller & Template section, I am sourcing my scripts using the same syntax they are. Do you not agree with the way they are doing things? Is there something fundamentally different about my use case that requires this kind of script sourcing to be done using different mechanisms?

I understand how using this kind of syntax:

<script>require('./app/app.js');</script>

Would "work", but also create globals and probably be undesirable. However, I'm not as clear on your second suggestion, regarding the use of a browser.js script. Can you give me a little bit more information about how I might use that approach in the specific use case illustrated in the example with which I provided you? Is there an existing example somewhere out there that illustrates the methodology you're trying to describe?

Thanks in advance...please let me know if I can provide any additional information or if there's anything at all I can do to help!

twolfson commented 8 years ago

Sure, here's some clarification/guidance

The require syntax won't create any globals but <script src=" will:

Next we have dependency management in the application. The tutorial you link to is following conventions that are ideal for a browser environment and doesn't use require in any of its browser code.

The gist of what's going on is:

If you want to continue using that paradigm, it's fine but it should also use the same dependency management system and probably not mix the 2 as it will issues (e.g. confused developers, 2 separate dependency trees)

In constrast, require handles both loading modules and dependency management. Here's an example which dodges <script src=", it uses import instead of require but the gist is the same. I should also mention that the tutorial you used is Angular@1 whereas Angular@2 has been released.

http://angular-tips.com/blog/2015/06/using-angular-1-dot-x-with-es6-and-webpack/

In other frameworks like React, they use require directly (with Browserify or Webpack for the browser) and don't have a separate dependency manager. Here's a simple example (see index.html and src):

https://github.com/toranb/react-gulp-browserify-example

With electron, we can directly require src/app.js from index.html instead of bundling first

twolfson commented 8 years ago

Updates to the actual issue: I have added a reference application to this repo based on the previous gist:

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

https://github.com/twolfson/karma-electron/tree/4.1.2/test/reference

The resulting values match our discussion so far (as we did the same work but outside of this repo):

https://github.com/twolfson/karma-electron/blob/4.1.2/test/reference/js/src-main.js

https://github.com/twolfson/karma-electron/blob/4.1.2/test/reference/js/require-main.js

The current unmodified behavior for electron with karma-electron are as follows. These are caused by karma using http:// instead of file://:

I think the default expected behavior would be (e.g. when <script src= is used):

Then we have the Node.js-like behavior (e.g. when <script>require is used):

In addition to this, there's the requested behavior which is to allow a custom context.html location -- we could allow this to be done via a customContextFile but I feel like that's cruel as it requires creation of extra files.

To solve these problems, I plan on doing the following:

I'm not certain of the electron namespacing or not but I will see what other repos do before deciding

twolfson commented 8 years ago

The first patch has been released in 5.0.0 (i.e. client.loadScriptsViaRequire). The __filenameOverride should be much quicker now that the majority is done

twolfson commented 8 years ago

We have added support for client.__filenameOverride in 5.1.0. @ThibaultFriedrich and @DanForbes The configuration you will probably want is:

module.exports = function (config) {
  config.set({
    client: {
      // Override top level `__filename` to be `/home/.../my-electron-app/index.html`
      //   where `__dirname` is `/home/.../my-electron-app`
      __filenameOverride: __dirname + '/path/to/index.html',
    }
  });
};

Thanks again for the bug report and clarification

friedrith commented 8 years ago

Hi, sorry for the silence, but I have just noticed that my github was using a trash email and so I have never been notified. I know it is quite late but I can provide more examples if needed. Thanks you @DanForbes and @twolfson for having kept this thread alive.

danforbes commented 8 years ago

Thanks for all the information and quick fix, @twolfson!