Search for module from app path when URL is not file protocol #9095

seanchas116 · 2017-04-03T11:30:14Z

This PR makes require()to search for modules from app directory when the URL is not file protocol (such as http, data or about:blank).

I think this would be more intuitive solution for #8425 and #8963 (comment).

kevinsawicki · 2017-04-03T15:15:39Z

lib/renderer/init.js

+    if (window.location.protocol !== 'chrome-devtools:') {
+      // Search for module under the app directory
+      // (remote.app doesn't work in devtools)
+      module.paths = module.paths.concat(Module._nodeModulePaths(electron.remote.app.getAppPath()))


remote.app.getAppPath() is a synchronous IPC call which would block the renderer process, could this be done instead with a command line flag like opener id or guest instance id is done?

kevinsawicki · 2017-04-03T17:53:30Z

spec/modules-spec.js

+      w.loadURL('about:blank')
+      const result = await w.webContents.executeJavaScript('typeof require("q").when')
+      assert.equal(result, 'function')
+      w.destroy()


This should be done in an afterEach block in case the assert fails, so the window does not leak across tests.

seanchas116 · 2017-04-04T00:42:27Z

Updated, thank you 👍

kevinsawicki

Left some very minor comments, looks good overall though 👍

kevinsawicki · 2017-04-11T23:02:05Z

spec/modules-spec.js

+    })
+
+    afterEach(() => {
+      w.destroy()


This should use closeWindow helper instead, see

electron/spec/api-browser-window-spec.js

Lines 84 to 86 in efa2850

afterEach(function () {

return closeWindow(w).then(function () { w = null })

})

for an example

kevinsawicki · 2017-04-11T23:03:02Z

spec/modules-spec.js

+      assert.equal(result, 'function')
+    })
+
+    afterEach(() => {


afterEach blocks usually go after the beforeEach but before the it blocks, mind moving it there?

kevinsawicki · 2017-04-12T16:12:31Z

atom/browser/api/atom_api_app.h

@@ -70,6 +70,8 @@ class App : public AtomBrowserClient::Delegate,
      std::unique_ptr<CertificateManagerModel> model);
 #endif

+  std::string GetAppPath();


I wonder if this might be better to store as a base::FilePath. That is what App::GetPath returns and it looks like on Windows base::FilePath will be converted to a std::wstring and it will be a std::string on other platforms.

poiru · 2017-04-13T10:15:55Z

atom/browser/api/atom_api_app.h

@@ -70,6 +70,8 @@ class App : public AtomBrowserClient::Delegate,
      std::unique_ptr<CertificateManagerModel> model);
 #endif

+  base::FilePath GetAppPath();


This could probably be const base::FilePath& GetAppPath().

kevinsawicki · 2017-04-20T17:49:50Z

Thanks for this @seanchas116 👍 🎆

seanchas116 · 2017-04-21T00:48:05Z

Thanks too 👍 🎉

lastmjs · 2017-05-10T05:46:21Z

@seanchas116 I'm just trying to understand how to use this in my app. I load an HTML file from a local HTTP server with Electron, and I would like to require my modules relative to the index.html file that I load. Will this help me achieve that? So far with my experimenting it doesn't seem to help, the app root is set to some strange location in node_modules/electron/bla/bla, which doesn't help get to any of the node_modules that my application would actually use. Any insight would be appreciated

seanchas116 · 2017-05-10T06:15:45Z

the app root is set to some strange location in node_modules/electron/bla/bla

The app path (which you can get by app.getAppPath()) would be usually the directory of the package.json you provide on Electron launch.
Probably Electron is launched without package.json?
If so you could provide package.json to require modules relative to it.

lastmjs · 2017-05-10T06:22:07Z

Thanks! I still have a couple questions:

The app path (which you can get by app.getAppPath()) would be usually the directory of the package.json you provide on Electron launch.

What do you mean by the package.json I provide on Electron launch? Here is my package.json:

{
  "name": "jfive-web-components",
  "version": "0.0.8",
  "description": "Web components for controlling hardware with the Johnny-Five JavaScript Robotics & IoT platform.",
  "scripts": {
    "test": "electron --enable-logging node_modules/scram-engine/main.js --entry-file test/index.html --window"
  },
  "repository": {
    "type": "git",
    "url": "git+https://github.com/scramjs/jfive-web-components.git"
  },
  "author": "Jordan Last <jordan.michael.last@gmail.com>",
  "license": "MIT",
  "bugs": {
    "url": "https://github.com/scramjs/jfive-web-components/issues"
  },
  "homepage": "https://github.com/scramjs/jfive-web-components#readme",
  "dependencies": {
    "johnny-five": "^0.10.10"
  },
  "devDependencies": {
    "jsverify": "^0.8.2",
    "mocha": "^3.3.0",
    "scram-engine": "git+ssh://git@github.com/scramjs/scram-engine.git",
    "sinon": "^2.2.0",
    "tape": "^4.6.3"
  }
}

When I log app.getAppPath() I get:

/home/lastmj/development/scramjs/jfive-web-components/node_modules/electron/dist/resources/default_app.asar

I think I would like it to be:

/home/lastmj/development/scramjs/jfive-web-components

Am I thinking about it incorrectly?

seanchas116 · 2017-05-10T06:36:24Z

Probably
electron --enable-logging node_modules/scram-engine/main.js --entry-file test/index.html --window
would have to be
electron . --enable-logging node_modules/scram-engine/main.js --entry-file test/index.html --window
to specify app path.

lastmjs · 2017-05-10T06:39:15Z

electron . doesn't work for me, but I just found in the commit app.setAppPath, and using that function it seems to be working, so thanks!

seanchas116 added 4 commits April 3, 2017 20:11

Add test for require to search under app dir

673f4f4

Search for module under the app directory

50c99e4

Fix JS style

d1212d4

Do not add search paths in devtools

001d03c

kevinsawicki reviewed Apr 3, 2017

View reviewed changes

seanchas116 added 3 commits April 4, 2017 09:08

Use beforeEach/afterEach

9cb6bc0

Pass app path as command line argument

4a7eec8

No extra linebreak

24fedb2

kevinsawicki suggested changes Apr 11, 2017

View reviewed changes

Fix afterEach

c77e07b

kevinsawicki reviewed Apr 12, 2017

View reviewed changes

Use base::FilePath

9d62b19

poiru reviewed Apr 13, 2017

View reviewed changes

Use const

ea6890a

kevinsawicki approved these changes Apr 20, 2017

View reviewed changes

kevinsawicki merged commit a004cad into electron:master Apr 20, 2017

kevinsawicki mentioned this pull request Apr 20, 2017

Add root describe to modules spec #9237

Merged

seanchas116 mentioned this pull request Apr 21, 2017

problems with requiring modules with "win.loadURL(data:text/html;charset=utf-8,${html})" #8425

Closed

lastmjs mentioned this pull request May 9, 2017

Look into getting rid of the require config scramjs/scram-engine#42

Closed

mgc mentioned this pull request May 11, 2017

app.setAppPath is not a function #9439

Closed

jezdesilva mentioned this pull request Jul 12, 2017

Third-party code failing in Require.js #9969

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search for module from app path when URL is not file protocol #9095

Search for module from app path when URL is not file protocol #9095

seanchas116 commented Apr 3, 2017

kevinsawicki Apr 3, 2017

kevinsawicki Apr 3, 2017

seanchas116 commented Apr 4, 2017

kevinsawicki left a comment

kevinsawicki Apr 11, 2017

kevinsawicki Apr 11, 2017

kevinsawicki Apr 12, 2017

poiru Apr 13, 2017

kevinsawicki commented Apr 20, 2017

seanchas116 commented Apr 21, 2017

lastmjs commented May 10, 2017

seanchas116 commented May 10, 2017 •

edited

Loading

lastmjs commented May 10, 2017

seanchas116 commented May 10, 2017

lastmjs commented May 10, 2017

	afterEach(function () {
	return closeWindow(w).then(function () { w = null })
	})

Search for module from app path when URL is not file protocol #9095

Search for module from app path when URL is not file protocol #9095

Conversation

seanchas116 commented Apr 3, 2017

kevinsawicki Apr 3, 2017

Choose a reason for hiding this comment

kevinsawicki Apr 3, 2017

Choose a reason for hiding this comment

seanchas116 commented Apr 4, 2017

kevinsawicki left a comment

Choose a reason for hiding this comment

kevinsawicki Apr 11, 2017

Choose a reason for hiding this comment

kevinsawicki Apr 11, 2017

Choose a reason for hiding this comment

kevinsawicki Apr 12, 2017

Choose a reason for hiding this comment

poiru Apr 13, 2017

Choose a reason for hiding this comment

kevinsawicki commented Apr 20, 2017

seanchas116 commented Apr 21, 2017

lastmjs commented May 10, 2017

seanchas116 commented May 10, 2017 • edited Loading

lastmjs commented May 10, 2017

seanchas116 commented May 10, 2017

lastmjs commented May 10, 2017

seanchas116 commented May 10, 2017 •

edited

Loading